-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Logs / Metrics UI] Switch to scopedHistory and enhance useLinkProps hook #61667
Conversation
…llowed (used by React-router's Prompt component)
Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm taking a look at this today.
@elasticmachine merge upstream |
@jasonrhodes Were there any changes needed on this one? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested this thoroughly and wasn't able to find anything wrong with it at all, I was pretty bummed to be honest. Merge away. Sorry again for the delay!
@elasticmachine merge upstream |
merge conflict between base and head |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
Summary
This ticket primarily fixes #58112 (which switches us over to using
scopedHistory
), but there have also been two enhancements to theuseLinkProps
hook. I'll explain the changes in detail below:Use
scopedHistory
We now use the
scopedHistory
history
instance that is passed in from platform as part of theAppMountParameters
.Use
navigateToApp
exclusively inuseLinkProps
Previously the
useLinkProps
hook had two logic forks, one for internal links and one for external links. Now that we're usingscopedHistory
, and becausenavigateToApp
just pushes tohistory
under the hood, we can just make use ofnavigateToApp
for allonClick
handlers.If navigating between two new platform applications there will be no page refresh (this is all NP apps now, not just navigating between logs and metrics).
Links to legacy apps will hard refresh, as expected.
Custom
<Prompt />
implementationThis is the change that requires the most explanation. Previously we were using the
<Prompt />
component fromreact-router
to warn the user if they were going to lose source configuration form changes.However, under the hood that react-router component uses
history.block
.history.block
is now banned withscopedHistory
, it will throw an Error and instead suggest the use of theonAppLeave
mount parameter.There are a couple of issues with using
onAppLeave
, A) it only handles switching between apps (not tabs of an app) and B) I found the behaviour a little buggy (sometimes the handler didn't fire switching apps).I've therefore implemented a little bit of context, and a custom
<Prompt />
component, to recreate the same behaviour the react-router component gave us.As we have a centralised location that handles the generation of our link props with
useLinkProps
it was easy to block navigation if needed there.Testing
Do links still work as expected? (Are the
href
attributes correct, encoded correctly etc. Do they work as expected when clicked?)If you make changes to the source configuration form, and then try to navigate away, is a warning displayed?