Refactoring nav links and header components #66685

myasonik · 2020-05-15T02:48:39Z

Summary

Made Header a functional component
- Shortened and simplified the file quite a bit
- Part of this also meant pushing observers to their farthest leaf component of where they're actually used reducing the rerender count, roughly, from 36 to 1 🎉
  - This also helped shorten the file
- Added tests (Add unit tests for new nav #65369)
Added href prop to ChromeNavLink
- Moved creation of href into to_nav_link.ts which consolidated how many times we had time figure it out at a few different leaf nodes
- In a roundabout way, simplified Header because a few places now didn't need to the full "EUI-ready" NavLink and could rely on the ChromeNavLink being passed around
Deleted a copy-paste of relativeToAbsolute() because imports are a thing and we don't need two of them
Renamed _collapsible_nav.scss to collapsible_nav.scss and used the fancy new Sass-in-JS import thing
Removed NavLink and RecentNavLink (Add unit tests for new nav #65369)
- The functions that used return these still exist in some form but instead of happening in Header then being passed around all over the place, it happens just before being passed into EUI so the types don't need to hang around
Made HeaderBreadcrumbs and HeaderNavControls functional components
- Just way easier to read and easier to manage the observers
Bloated NavDrawer a little but it's ready to be deleted as soon as the old nav is removed
Removed truncateRecentItemLabel because it wasn't working before my nav changes even went in and the new nav doesn't need it anyways so it'd be only for the old nav (Add unit tests for new nav #65369)

Closes #65369

Checklist

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

src/core/server/legacy/plugins/get_nav_links.ts

myasonik · 2020-05-15T02:59:31Z

@pgayvallet @joshdover Hoping one/both of y'all can take a look at this while I start working on these tests. Hopefully this refactor is going in the right direction. Don't really plan on touching much more other than test files.

Full description of changes in the description.

src/core/public/chrome/ui/header/collapsible_nav.tsx

pgayvallet

A few NITs and question, overall looking good. Thanks for the migration to FC!

pgayvallet · 2020-05-20T06:41:29Z

src/core/public/chrome/nav_links/to_nav_link.ts

    ...(isLegacyApp(app)
-      ? {}
+      ? {
+          href: url && !url.startsWith(app.subUrlBase!) ? url : baseUrl,
+        }
      : {
-          url: relativeToAbsolute(appendAppPath(baseUrl, app.defaultPath)),
+          href: url,
+          url,
        }),


Moving the url logic to this function instead of the component makes sense, but I'm not sure to understand why href was introduced instead of just using the existing url and baseUrl here?

Mostly to align with EUI prop names otherwise it's one more prop I have to alias at the component level.

Also has a couple side benefits:

Makes sure that nothing breaks if something else was low key relying on url being a certain thing at a certain time so makes the refactor a little safer (can refactor one thing at a time)

Also makes clear what this will be used for (URLs can be constructed in a lot of different ways so knowing exactly what you're getting can be a bit of a toss up. With href you know exactly what the ultimate purpose of the prop is.)

pgayvallet · 2020-05-20T06:43:12Z

src/core/public/chrome/ui/header/header.tsx

  basePath: HttpStart['basePath'];
  isLocked$: Rx.Observable<boolean>;
  navType$: Rx.Observable<NavType>;
+  loadingCount$: ReturnType<HttpStart['getLoadingCount$']>;


NIT: would just use Observable<number> here.

Why's that? I thought it would be nice to have a single source of truth on the type.

IDK, either way, TS compilation would fail if HttpStart['getLoadingCount$'] type changes (just not in the same file), and I find it way more readable. But it's a personal option, you can keep it like that.

(The same way I would replace HttpStart['basePath'] by it's IBasePath actual type)

👍 I'll keep it like this for now then. If other people have opinions on this, would be nice to come to a consensus on what we should prefer in the Kibana repo.

src/core/public/chrome/ui/header/nav_link.tsx

src/core/server/legacy/plugins/get_nav_links.ts

elasticmachine · 2020-05-20T16:12:07Z

Pinging @elastic/kibana-platform (Team:Platform)

myasonik · 2020-05-22T03:14:19Z

retest

pgayvallet · 2020-05-26T19:26:21Z

Ack: will review tomorrow

pgayvallet

Some nits and comments, overall LGTM

pgayvallet · 2020-05-27T06:33:40Z

src/core/public/chrome/ui/header/collapsible_nav.tsx

+      dataTestSubj: 'collapsibleNavAppLink',
+      navigateToApp,
+      onClick: closeNav,
+      ...(needsIcon && { basePath }),


Probably minor, but I find it a little misleading that the addition of the icon in createEuiListItem is only based on the presence or not of the basePath input.

pgayvallet · 2020-05-27T06:36:51Z

src/core/public/chrome/ui/header/collapsible_nav.tsx

+            listItems={recentlyAccessed.map(link => {
+              // TODO #64541
+              // Can remove icon from recent links completely
+              const { iconType, ...hydratedLink } = createRecentNavLink(link, navLinks, basePath);


NIT: adding a omitIcon = false param to createRecentNavLink seems a little better than spread exclusion?

src/core/public/chrome/ui/header/header.test.tsx

pgayvallet · 2020-05-27T06:45:07Z

src/core/public/chrome/ui/header/header_logo.tsx

+export function HeaderLogo({ href, navigateToApp, ...observables }: Props) {
+  const forceNavigation = useObservable(observables.forceNavigation$, false);
+  const navLinks = useObservable(observables.navLinks$, []);


NIT: not sure to see a real upside in using a rest parameter here, but I'm fine either way

src/core/public/chrome/ui/header/nav_link.tsx

snide

Ran through the sass changes, which were minimal. File move looks good.

lukeelmers

App arch change LGTM -- seems this just reordered some props that were included in our generated markdown documentation.

kibanamachine · 2020-05-29T03:01:46Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 8920544

History

💔 Build #50456 failed 728bd7a
💔 Build #50212 failed 125735c
💔 Build #50192 failed 1959f1e
💔 Build #50133 failed 3f868d5429c186932e3fc0fc0f273dd3ff9a8651
💚 Build #49444 succeeded 37ac9e4

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

Co-authored-by: spalger <[email protected]> # Conflicts: # docs/development/plugins/data/public/kibana-plugin-plugins-data-public.querystringinput.md # docs/development/plugins/data/public/kibana-plugin-plugins-data-public.searchbar.md # src/core/public/chrome/chrome_service.tsx # src/core/public/chrome/ui/header/__snapshots__/collapsible_nav.test.tsx.snap # src/core/public/chrome/ui/header/collapsible_nav.test.tsx # src/core/public/chrome/ui/header/collapsible_nav.tsx # src/core/public/chrome/ui/header/header.tsx # src/core/public/chrome/ui/header/header_logo.tsx # src/core/public/chrome/ui/header/nav_drawer.tsx # src/plugins/data/public/public.api.md

Co-authored-by: spalger <[email protected]>

kibanamachine · 2020-06-01T14:28:44Z

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

Co-authored-by: spalger <[email protected]> Co-authored-by: Josh Dover <[email protected]> Co-authored-by: Elastic Machine <[email protected]>

first draft refactoring nav link created

1c67d67

myasonik commented May 15, 2020

View reviewed changes

src/core/server/legacy/plugins/get_nav_links.ts Outdated Show resolved Hide resolved

joshdover reviewed May 18, 2020

View reviewed changes

src/core/public/chrome/ui/header/collapsible_nav.tsx Outdated Show resolved Hide resolved

myasonik mentioned this pull request May 18, 2020

Adds link for Cloud deployment settings #66486

Merged

7 tasks

Michail Yasonik added 2 commits May 19, 2020 20:15

fixing tests

f63f377

api update

a94ae19

pgayvallet reviewed May 20, 2020

View reviewed changes

finishing up tests, fixing PR feedback...

196639b

myasonik added Feature:Header Work related to the header section of the Kibana app. release_note:fix Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// v7.9.0 v8.0.0 labels May 20, 2020

Michail Yasonik added 3 commits May 21, 2020 15:54

stubbing test autogenerated ids

06ac35e

Merge remote-tracking branch 'upstream/master' into nav-refactor

6d4aadc

header snapshot fix

37ac9e4

myasonik marked this pull request as ready for review May 22, 2020 14:27

myasonik requested review from a team as code owners May 22, 2020 14:27

pgayvallet approved these changes May 27, 2020

View reviewed changes

PR nit resolution

3b3b663

snide approved these changes May 27, 2020

View reviewed changes

lukeelmers approved these changes May 27, 2020

View reviewed changes

myasonik force-pushed the nav-refactor branch from 3f868d5 to 75c4521 Compare May 27, 2020 23:06

myasonik requested a review from watson as a code owner May 27, 2020 23:06

spalger force-pushed the nav-refactor branch from 75c4521 to 1959f1e Compare May 27, 2020 23:14

spalger removed request for a team May 27, 2020 23:14

myasonik removed the request for review from watson May 27, 2020 23:40

Michail Yasonik and others added 5 commits May 27, 2020 22:22

fixing types

125735c

Merge branch 'master' of github.com:elastic/kibana into pr/66685

eddd5e2

fix eslint violations

728bd7a

jest test fix

d6f21ba

Merge remote-tracking branch 'upstream/master' into nav-refactor

8920544

myasonik merged commit fce016c into elastic:master May 29, 2020

myasonik deleted the nav-refactor branch May 29, 2020 14:26

myasonik added the v7.8.0 label May 29, 2020

This was referenced May 29, 2020

[7.x] Refactoring nav links and header components (#66685) #67771

Merged

[7.8] Refactoring nav links and header components (#66685) #67772

Merged

myasonik pushed a commit that referenced this pull request May 29, 2020

[7.x] Refactoring nav links and header components (#66685) (#67771)

4415b35

Co-authored-by: spalger <[email protected]>

kibanamachine added backport missing Added to PRs automatically when the are determined to be missing a backport. and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Jun 1, 2020

myasonik pushed a commit that referenced this pull request Jun 1, 2020

[7.8] Refactoring nav links and header components (#66685) (#67772)

f4d6c23

Co-authored-by: spalger <[email protected]> Co-authored-by: Josh Dover <[email protected]> Co-authored-by: Elastic Machine <[email protected]>

Refactoring nav links and header components #66685

Refactoring nav links and header components #66685

Uh oh!

Conversation

myasonik commented May 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

Uh oh!

myasonik commented May 15, 2020

Uh oh!

Uh oh!

pgayvallet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elasticmachine commented May 20, 2020

Uh oh!

myasonik commented May 22, 2020

Uh oh!

pgayvallet commented May 26, 2020

Uh oh!

pgayvallet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

snide left a comment

Choose a reason for hiding this comment

Uh oh!

lukeelmers left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented May 29, 2020

💚 Build Succeeded

History

Uh oh!

kibanamachine commented Jun 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

myasonik commented May 15, 2020 •

edited

Loading