[InfraUI] Infrastructure navigation changes #32892

Kerry350 · 2019-03-11T14:16:56Z

Summary

Implements the outstanding work on #27916 (Change primary navigation, and add sub navigation and Move node type filter navigation to query/filter element).

This is how the changes look:

Uses a standard nested pattern whereby /infrastructure mounts the index page, and then nested routes mount.

Also some very small changes to <DocumentTitle /> to better support nesting (specifically accessing the previousTitle).

I've left the Metrics explorer tab in for now as it helps with testing, and will be implemented for real in #28027. Prior to merging I can either just comment that tab out until it's worked on, or remove it.

Testing

Try clicking the primary navigation tabs, ensure the correct page renders.
Try clicking the hosts / Kubernetes / Docker sub-navigation, ensure this filters by the correct nodeType.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
~~[ ] Documentation was added for features that require explanation or tutorials~~
~~[ ] Unit or functional tests were updated or added to match the most common scenarios~~
This was checked for keyboard-only and screenreader accessibility

Add routed_tabs component Amend routing (use an index and nested routes) and reorganise directory structure Shift hosts / kubernetes / docker selection to subnavigation Change path Amend existing URLs Use proper types Amend document_title component Reorganise title / source config button / info text Maintain compatibility with old URLs

elasticmachine · 2019-03-11T14:16:58Z

Pinging @elastic/infrastructure-ui

elasticmachine · 2019-03-11T14:30:40Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-03-11T16:01:25Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

weltenwort

Looks really good, I like it!

Especially the Route usage in RoutedTabs looks very elegant to me, good job 👍

Aside from that I left a few comments inline and also noticed the following:

There seems to be some z-index problems with the auto-completion popup (again 😉):

Not sure if that is related to the styles of the popup itself being broken. Maybe that undid previous z-index fixes. We'll have to check if 7.0 is affected too.

I also happened to notice, that the DocumentTitle component doesn't seem to correctly deal with the title prop being updated, which leads to incorrect titles parts of it are loaded asynchronously (as is the case one the node details page):

That is probably not caused by this PR, so we could just as well fix that separately.

weltenwort · 2019-03-11T16:26:24Z

x-pack/plugins/infra/public/components/waffle/waffle_node_type_switcher.tsx

-          />
-        </EuiKeyPadMenuItemButton>
-      </EuiKeyPadMenu>
+      <EuiButtonGroup


I know the EUI docs recommend the small button size for groups, but how about setting buttonSize="m" on this one? To my eye the heights and spacing in

look a lot more harmonious than with the default size s

Yep, I agree. I have changed this, but there is a little bit of an issue with the EUI typings for this component.

The <EuiButtonGroup /> component allows "m" for buttonSize. But, that component in it's tree renders a <EuiButton /> which only allows "s" and "l". There is a mismatch here; I can either leave the change in, we ignore the console warning for now, and I file a ticket against EUI. Or don't make the change so we don't have an error, file against EUI, change it once the EUI side is changed.

Indeed, I hadn't noticed that. I would say we should file an issue in EUI (or a PR, given the simplicity of the change) and leave it in for now.

x-pack/plugins/infra/public/components/waffle/waffle_node_type_switcher.tsx

x-pack/plugins/infra/public/components/navigation/routed_tabs.tsx

x-pack/plugins/infra/public/components/waffle/waffle_node_type_switcher.tsx

x-pack/plugins/infra/public/register_feature.ts

x-pack/plugins/infra/public/pages/infrastructure/index.tsx

…a-navigation

Kerry350 · 2019-03-12T14:36:10Z

@weltenwort Thanks for the review 😄

There seems to be some z-index problems with the auto-completion popup (again 😉):

This is fixed now (good spot!). The original fix boosted the toolbar in it's entirety to a higher z-index than the page content, which worked for the original issue. Here, because of the layout changes, we have two rows so that fix isn't sufficient. I've added the same z-index boost to the suggestions panel to ensure it doesn't interfere with the secondary row.

I also happened to notice, that the DocumentTitle component doesn't seem to correctly deal with the title prop being updated ... That is probably not caused by this PR, so we could just as well fix that separately.

Oops, this was indeed caused by this PR and making improvements in that component. This is fixed now.

Kerry350 · 2019-03-12T14:54:47Z

@weltenwort I've responded to all feedback now. There are two things I couldn't fully resolve, regarding EUI components.

elasticmachine · 2019-03-12T15:18:15Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…a-navigation

elasticmachine · 2019-03-13T12:58:11Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

weltenwort

Nice fixes, we're almost there. As to the "Metrics Explorer" tab, I guess we should hide it (comment it out) for now before merging?

x-pack/plugins/infra/public/components/navigation/routed_tabs.tsx

…a-navigation

Kerry350 · 2019-03-14T12:49:13Z

@weltenwort

I only now noticed there might be an accessibility issue with nesting the inside of the

Very good point. I've changed this 👍

This should be good to go now (the metrics-explorer tab is commented out for now). I'll handle the two EUI bits in followup PRs as agreed.

elasticmachine · 2019-03-14T13:37:07Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

weltenwort

Very nice work! One last suggestion below, but I think this is definitely a great improvement. I'd love to imitate some of that on logs page. 😉

x-pack/plugins/infra/public/components/navigation/routed_tabs.tsx

…a-navigation

elasticmachine · 2019-03-18T14:01:31Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* Reorganise primary and sub navigation of Infrastructure Add routed_tabs component Amend routing (use an index and nested routes) and reorganise directory structure Shift hosts / kubernetes / docker selection to subnavigation Change path Amend existing URLs Use proper types Amend document_title component Reorganise title / source config button / info text Maintain compatibility with old URLs * Remove dead code * Bump z-index on suggestions panel * Ensure documentTitle responds to updates * Use the same noop function and remove disabled option * Use buttonSize="m" and add "Hosts" translation back * Remove all "Home" wording * Remove unused translations with script * Don't nest <Link /> within a tab * Comment out metrics-explorer tab until needed * Don't create duplicate History entries

Kerry350 added review Feature:Metrics UI Metrics UI feature v8.0.0 Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.2.0 labels Mar 11, 2019

Kerry350 self-assigned this Mar 11, 2019

Kerry350 requested review from skh and weltenwort March 11, 2019 14:16

Kerry350 added the [zube]: In Review label Mar 11, 2019

Remove dead code

9f27c82

weltenwort removed the request for review from skh March 11, 2019 16:09

weltenwort reviewed Mar 11, 2019

View reviewed changes

Kerry350 added 7 commits March 12, 2019 13:03

Merge remote-tracking branch 'upstream/master' into 27916-change-infr…

39b0077

…a-navigation

Bump z-index on suggestions panel

037518f

Ensure documentTitle responds to updates

3624017

Use the same noop function and remove disabled option

1f0078f

Use buttonSize="m" and add "Hosts" translation back

cd5542d

Remove all "Home" wording

7663a1f

Remove unused translations with script

32d061c

Merge remote-tracking branch 'upstream/master' into 27916-change-infr…

c1d22e4

…a-navigation

weltenwort reviewed Mar 13, 2019

View reviewed changes

x-pack/plugins/infra/public/components/navigation/routed_tabs.tsx Outdated Show resolved Hide resolved

Merge remote-tracking branch 'upstream/master' into 27916-change-infr…

ea80a16

…a-navigation

Kerry350 added 2 commits March 14, 2019 12:31

Don't nest <Link /> within a tab

99ce1b4

Comment out metrics-explorer tab until needed

3999ab2

weltenwort approved these changes Mar 15, 2019

View reviewed changes

x-pack/plugins/infra/public/components/navigation/routed_tabs.tsx Outdated Show resolved Hide resolved

Kerry350 added 3 commits March 18, 2019 10:56

Merge remote-tracking branch 'upstream/master' into 27916-change-infr…

b59424f

…a-navigation

Merge remote-tracking branch 'upstream/master' into 27916-change-infr…

d4a0217

…a-navigation

Don't create duplicate History entries

cd04728

Kerry350 merged commit 4b2c2bd into elastic:master Mar 18, 2019

zube bot added [zube]: Done and removed [zube]: In Review labels Mar 18, 2019

Kerry350 mentioned this pull request Mar 18, 2019

[7.x] [InfraUI] Infrastructure navigation changes (#32892) #33412

Merged

ruflin removed the [zube]: Done label Mar 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[InfraUI] Infrastructure navigation changes #32892

[InfraUI] Infrastructure navigation changes #32892

Kerry350 commented Mar 11, 2019 •

edited

Loading

elasticmachine commented Mar 11, 2019

elasticmachine commented Mar 11, 2019

elasticmachine commented Mar 11, 2019

weltenwort left a comment

weltenwort Mar 11, 2019

Kerry350 Mar 12, 2019 •

edited

Loading

weltenwort Mar 13, 2019

Kerry350 commented Mar 12, 2019

Kerry350 commented Mar 12, 2019

elasticmachine commented Mar 12, 2019

elasticmachine commented Mar 13, 2019

weltenwort left a comment

Kerry350 commented Mar 14, 2019

elasticmachine commented Mar 14, 2019

weltenwort left a comment

elasticmachine commented Mar 18, 2019

[InfraUI] Infrastructure navigation changes #32892

[InfraUI] Infrastructure navigation changes #32892

Conversation

Kerry350 commented Mar 11, 2019 • edited Loading

Summary

Testing

Checklist

elasticmachine commented Mar 11, 2019

elasticmachine commented Mar 11, 2019

💔 Build Failed

elasticmachine commented Mar 11, 2019

💔 Build Failed

weltenwort left a comment

Choose a reason for hiding this comment

weltenwort Mar 11, 2019

Choose a reason for hiding this comment

Kerry350 Mar 12, 2019 • edited Loading

Choose a reason for hiding this comment

weltenwort Mar 13, 2019

Choose a reason for hiding this comment

Kerry350 commented Mar 12, 2019

Kerry350 commented Mar 12, 2019

elasticmachine commented Mar 12, 2019

💔 Build Failed

elasticmachine commented Mar 13, 2019

💚 Build Succeeded

weltenwort left a comment

Choose a reason for hiding this comment

Kerry350 commented Mar 14, 2019

elasticmachine commented Mar 14, 2019

💚 Build Succeeded

weltenwort left a comment

Choose a reason for hiding this comment

elasticmachine commented Mar 18, 2019

💚 Build Succeeded

Kerry350 commented Mar 11, 2019 •

edited

Loading

Kerry350 Mar 12, 2019 •

edited

Loading