-
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
[uptime] Fix tab focus issue for settings page #87466
Conversation
@elasticmachine merge upstream |
Pinging @elastic/uptime (Team:uptime) |
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.
This looks great and works when testing. Just a question about the useMemo
dependencies.
dispatch(getConnectorsAction.get()); | ||
setAddFlyoutVisibility(false); | ||
focusInput(); | ||
}, | ||
}), | ||
// eslint-disable-next-line react-hooks/exhaustive-deps |
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.
Not a part of this PR, but why are we disabling exhaustive deps? It's generally not recommended by the hooks team.
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 also don't have much context, this particular piece was added by alerting team.
As far i remember, it was rendering component twice , which was causing some flickering behaviours.
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 played around with adding the dependencies, and I did not see a flicker. I console.log'd inside the useMemo to make sure it was only ever firing once on component mount, and played around with touching all the buttons inside the flyout. Perhaps recent refactors have made this no longer necessary?
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.
in any case, we can do that as follow up. I don't want to increase the scope of this PR.
const ConnectorAddFlyout = useMemo( | ||
() => | ||
getAddConnectorFlyout({ | ||
consumer: 'uptime', | ||
onClose: () => setAddFlyoutVisibility(false), | ||
onClose: () => { | ||
dispatch(getConnectorsAction.get()); |
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.
What does this bit of logic do? Unsure of how to test its move from useEffect to onClose.
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.
so when flyout closes, it's refetching all the connector actions, that are displayed in combo box options in settings page.
💛 Build succeeded, but was flaky
Test FailuresChrome X-Pack UI Plugin Functional Tests.x-pack/test/plugin_functional/test_suites/resolver.Resolver test app when the user is interacting with the node with ID: secondChild when the user hovers over the primary button when the user has clicked the primary button (which selects the node.) should render as expectedStandard Out
Stack Trace
Metrics [docs]Async chunks
Distributable file count
History
To update your PR or re-run it, just comment with: |
Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
Summary
Resolves: #87406
Settings tabs will properly focused on switch