Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Routing features (loc changes, NavigationLock) #26919

Closed
wants to merge 17 commits into from

Conversation

guardrex
Copy link
Collaborator

@guardrex guardrex commented Sep 2, 2022

Addresses #26364

Internal Review Topic (links to sections):

💥🚑🚒🚓

Ignore the doc build failure. We'll figure that out separately.

Notes

These are numbered to facilitate easy feedback responses.

  1. I take access to the API after RC1 is released, so this is untested locally. I may have wonky bits 💥 at this point due to that.
  2. Structure/layout of the content on the PR:
    • The nav options are covered early ... right next to the early, existing NavigateTo coverage.
    • The existing content on handling loc changes at the Router level should probably be covered first, as it's broader, app-level behavior.
    • The new section for handling loc changes in components follows.
  3. The coverage seeks to address the most common use cases. Do we need to expand for additional anticipated common use cases? It's best if you shoot the code example(s) to me for what you have in mind. Either you or I can set up code as fully-working/cut-'n-paste examples. I recommend we take that approach as much as possible.
  4. I cross-link the BasicTestApp examples but don't think we want to show something like these in general coverage. Devs can explore them (and try them locally) if they wish after following the links.
  5. It looks like handler disposal is automatic. Even if that's correct, is there anything in particular to call out beyond what I say in the text about it?
  6. Outside of the nice ConfirmExternalNavigation feature, what's better about using NavigationLock over just registering a handler and not using the NavigationLock component?
  7. Per MS style guidelines, we describe 'selected' buttons, not 'clicked' buttons.

@guardrex guardrex mentioned this pull request Sep 2, 2022
8 tasks
@guardrex guardrex marked this pull request as ready for review September 6, 2022 12:08
@guardrex guardrex closed this Sep 6, 2022
@guardrex guardrex reopened this Sep 6, 2022
@guardrex guardrex closed this Sep 6, 2022
@guardrex guardrex reopened this Sep 6, 2022
@guardrex guardrex closed this Sep 6, 2022
@guardrex guardrex reopened this Sep 6, 2022
@guardrex
Copy link
Collaborator Author

guardrex commented Sep 6, 2022

@Rick-Anderson ... I might need some SiteHelp™ on this one. Other PRs are building. I can't get a build report. I don't know if PR content is choking the build system or if the build system itself has something weird going on.

Error: Did not find OPS status check after waiting for 5 minutes. If it shows 'Expected — Waiting for status to be reported', close and reopen the pull request to trigger a build.
Error: Cannot read property 'state' of undefined

Strange to break where it did. The first commit where things went sideways is a trivial update ...

9346435

@guardrex
Copy link
Collaborator Author

guardrex commented Sep 6, 2022

@MackinnonBuck ... I think I have the disposal set up correctly now.

... well ... one more commit 👇 to remove the var. It might be ok now.

Co-authored-by: Mackinnon Buck <mackinnon.buck@gmail.com>
@guardrex
Copy link
Collaborator Author

guardrex commented Sep 6, 2022

Ok ... getting there. I went with ...

Handle (prevent) location changes

... for the section name, since preventing nav is a form of handling nav. It will be easy for devs to spot it as a feature like this in the topic's ToC sidebar.

UPDATE: mmmmmmmmmmmmm ... 🤔 ....... maybe not. That makes it sound like that's all you can do ... that preventing nav is the only way to handle nav here. Let me try again! brb

UPDATE: I think ur right. I played with several heading options. I came around to your suggestion. It's on the next commit 👇.

Copy link
Member

@MackinnonBuck MackinnonBuck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@guardrex
Copy link
Collaborator Author

guardrex commented Sep 6, 2022

Thanks, @MackinnonBuck!

I'll get up with Rick on the failing build. It looks like a SiteHelp issue will need to be opened internally to find out what's wrong with this PR. Anyway ... I need to 🛌😴 on it for one night anyway and make final grammar/style changes.

@Rick-Anderson
Copy link
Contributor

Thanks, @MackinnonBuck!

I'll get up with Rick on the failing build. It looks like a SiteHelp issue will need to be opened internally to find out what's wrong with this PR. Anyway ... I need to 🛌😴 on it for one night anyway and make final grammar/style changes.

Lets give it a few hours. Build was down last week. If it's not building by tomorrow 4 AM, @mention_ME and I'll open a case.

Copy link
Member

@MackinnonBuck MackinnonBuck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may also want to document the conclusion we recently reached in dotnet/aspnetcore#43725. The ConfirmExternalNavigation parameter will do nothing if the user directly changes the URL in the address bar to an external site without interacting with the page first.

aspnetcore/blazor/fundamentals/routing.md Outdated Show resolved Hide resolved
guardrex and others added 3 commits September 7, 2022 05:24
Co-authored-by: Mackinnon Buck <mackinnon.buck@gmail.com>
@guardrex
Copy link
Collaborator Author

guardrex commented Sep 7, 2022

In an effort to get this merged, I'm going to try a fresh branch/PR. This all started happening on a trivial content commit, and other PRs are building just fine. I don't see any blown markdown. This PR just can't shake off its 😈 gremlins 😈.

@guardrex
Copy link
Collaborator Author

guardrex commented Sep 7, 2022

Moved to the 🎉 successfully building 🍻 #26947.

@guardrex guardrex closed this Sep 7, 2022
@guardrex guardrex deleted the guardrex/blazor-navigation-events branch September 7, 2022 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants