[Workplace Search] Convert Settings pages to new page template#102445
[Workplace Search] Convert Settings pages to new page template#102445cee-chen merged 6 commits intoelastic:masterfrom
Conversation
cee-chen
added
release_note:skip
+ update main WS nav
cee-chen
force-pushed
the
kibana-page-template-ws-2
branch
from
a226623 to
d99d518
Compare
💚 Build SucceededMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
yakhinvadim
left a comment
yakhinvadim
left a comment
There was a problem hiding this comment.
This is so much cleaner! Looks great! 🎉
| GROUPS_PATH, | ||
| ORG_SETTINGS_PATH, | ||
| } from '../../routes'; | ||
| import { useSettingsSubNav } from '../../views/settings/components/settings_sub_nav'; |
There was a problem hiding this comment.
Optional: I associate "use" with React hooks. What do you think about renaming this to getSettingsSubNav? Totally optional, this definitely works. If you agree feel free to do it in a different PR!
There was a problem hiding this comment.
++ I thought the same thing when I saw it.
There was a problem hiding this comment.
It is a React hook actually! And (unfortunately) it does has to be one/have the use prefix in order for the function itself to use hooks, e.g. useRouteMatch, useValues, etc. - we'll get errors and crashes otherwise. I did try a simple getXNav but no dice, couldn't get it to work 😢
There was a problem hiding this comment.
Also since we're discussing this here, I wanted to mention some unfortunately mildly annoying shenanigans with the useXSubNav hook pattern:
- Hooks can't ever be called conditionally. I tried to do things like this (for performance):
items: useRouteMatch(SOURCES_PATH) ? useSourceSubNav() : []But I got React errors because you're supposed to always call hooks in a set order. So I fell back to the hook itself returning early/undefined instead.
- You can't use a hook within a hook/callback - I saw that the Observability folks were using useMemo to memoize their side nav items/reduce rerenders and wanted to copy that, but unfortunately we can't nest a
useXSubNavwithin auseMemo. It's probably not a huge deal and our app is still super responsive, but it definitely feels like it would be nice to get around this.
The primary issue is, without it being a useSubNav fn, we can't use the Kea useValues hooks. And unfortunately we do need the actual useValues hook, we can't simply do SourceLogic.values.contentSource.id - it crashes/errors on pages where SourceLogic isn't mounted.
There are a few ways I can think of to work around these limitations, but they're potentially complicated and require adding new infrastructure (e.g.: hook checks within the main nav itself but not the sub navs? useParams? an always-mounted logic file just for the nav which stores source/group IDs so we can avoid useValues?) and I tried to make this conversion as simple as possible for now.
I'm definitely interested in coming back to see if we can avoid hooks entirely for the sub navs, or someone else is totally welcome to look, but also we may be better off punting until 7.15 or until it becomes a major pain point.
There was a problem hiding this comment.
I agree that punting is fine for now.
scottybollinger
left a comment
scottybollinger
left a comment
There was a problem hiding this comment.
Just adding my $0.02.
This is awesome.
🎉
…ic#102445) * Convert Settings > Customize to new page template * Convert Settings > oAuth application to new page template + DRY form wrappers, update test * Convert Settings > Connectors to new page template * Convert source config view to new page template * Convert Settings subnav to EuiSideNav format + update main WS nav * Update routers
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
…) (#102532) * Convert Settings > Customize to new page template * Convert Settings > oAuth application to new page template + DRY form wrappers, update test * Convert Settings > Connectors to new page template * Convert source config view to new page template * Convert Settings subnav to EuiSideNav format + update main WS nav * Update routers Co-authored-by: Constance <constancecchen@users.noreply.github.com>
4 participants
Summary
Follow up to #102170 - converts more Workplace Search pages to the new KibanaPageTemplate. I'm attempting to break up the WS layout conversion into smaller, easier to review chunks.
This PR handles the Settings pages and sub-nav. As always, follow along by commit (and turn off whitespace diffs)!
Screencaps
Checklist