[Workplace Search] Convert Settings pages to new page template #102445

cee-chen · 2021-06-17T04:58:41Z

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

Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

+ DRY form wrappers, update test

+ update main WS nav

kibanamachine · 2021-06-17T08:07:06Z

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`enterpriseSearch`	2.1MB	2.1MB	-941.0B

History

💚 Build #131939 succeeded a226623

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

yakhinvadim

This is so much cleaner! Looks great! 🎉

yakhinvadim · 2021-06-17T13:46:08Z

x-pack/plugins/enterprise_search/public/applications/workplace_search/components/layout/nav.tsx

@@ -19,6 +19,7 @@ import {
  GROUPS_PATH,
  ORG_SETTINGS_PATH,
 } from '../../routes';
+import { useSettingsSubNav } from '../../views/settings/components/settings_sub_nav';


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!

++ I thought the same thing when I saw it.

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 😢

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 useXSubNav within a useMemo. 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.

I agree that punting is fine for now.

scottybollinger

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

kibanamachine · 2021-06-17T16:39:04Z

💚 Backport successful

Status	Branch	Result
✅	7.x

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>

cee-chen added 4 commits June 16, 2021 21:50

Convert Settings > Customize to new page template

9240f4b

Convert Settings > oAuth application to new page template

cc36617

+ DRY form wrappers, update test

Convert Settings > Connectors to new page template

0fc182b

Convert source config view to new page template

cfe6d65

cee-chen added release_note:skip Skip the PR/issue when compiling release notes v7.14.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Jun 17, 2021

cee-chen requested a review from a team June 17, 2021 04:58

cee-chen added 2 commits June 16, 2021 22:59

Convert Settings subnav to EuiSideNav format

2fbe05b

+ update main WS nav

Update routers

d99d518

cee-chen force-pushed the kibana-page-template-ws-2 branch from a226623 to d99d518 Compare June 17, 2021 05:59

yakhinvadim approved these changes Jun 17, 2021

View reviewed changes

scottybollinger approved these changes Jun 17, 2021

View reviewed changes

cee-chen merged commit 0e14cef into elastic:master Jun 17, 2021

cee-chen deleted the kibana-page-template-ws-2 branch June 17, 2021 16:36

kibanamachine mentioned this pull request Jun 17, 2021

[7.x] [Workplace Search] Convert Settings pages to new page template (#102445) #102532

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Workplace Search] Convert Settings pages to new page template #102445

[Workplace Search] Convert Settings pages to new page template #102445

cee-chen commented Jun 17, 2021

kibanamachine commented Jun 17, 2021

yakhinvadim left a comment

yakhinvadim Jun 17, 2021

scottybollinger Jun 17, 2021

cee-chen Jun 17, 2021 •

edited

Loading

scottybollinger Jun 17, 2021

cee-chen Jun 17, 2021 •

edited

Loading

yakhinvadim Jun 17, 2021

scottybollinger left a comment

kibanamachine commented Jun 17, 2021

[Workplace Search] Convert Settings pages to new page template #102445

[Workplace Search] Convert Settings pages to new page template #102445

Conversation

cee-chen commented Jun 17, 2021

Summary

Screencaps

Checklist

kibanamachine commented Jun 17, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

yakhinvadim left a comment

Choose a reason for hiding this comment

yakhinvadim Jun 17, 2021

Choose a reason for hiding this comment

scottybollinger Jun 17, 2021

Choose a reason for hiding this comment

cee-chen Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

scottybollinger Jun 17, 2021

Choose a reason for hiding this comment

cee-chen Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

yakhinvadim Jun 17, 2021

Choose a reason for hiding this comment

scottybollinger left a comment

Choose a reason for hiding this comment

kibanamachine commented Jun 17, 2021

💚 Backport successful

cee-chen Jun 17, 2021 •

edited

Loading

cee-chen Jun 17, 2021 •

edited

Loading