Skip to content
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

[LogsUI] [InfraUI] Turn source configuration into a tab and standardize the main navigation #42243

Merged
merged 31 commits into from
Aug 6, 2019

Conversation

Kerry350
Copy link
Contributor

Summary

This ticket implements #39071, and also implements the Infra focussed concerns of #150 by changing the main navigation to align with SIEM / APM.

This implements all of the outlined acceptance criteria, there's also one extra addition in that the locations we had a "Change configuration" button that used to open the flyout (no data states for example) now navigate to the settings page. Those changes are all in this commit.

Screenshot 2019-07-30 12 20 08

One thing to note is that I've had to disable some functional tests for now. I am still currently looking in to those, but as that was the only thing blocking this PR being opened and reviewed I thought it best to disable them. **

===============================================

** The issues I'm seeing in the functional tests are very odd. For example, in one of the tests this assertion fails:

const logStreamEntries = await infraLogStream.getStreamEntries();
expect(logStreamEntries.length).to.be.greaterThan(0);

As logStreamEntries.length returns with a length of 0. However, when I check the screenshot output in the failures folder the elements are rendered in full. Similarly, if I check the HTML output in the failure_debug folder the DOM elements with the correct data-test-subj all exist. So I can't possibly see how they're 0.

Even more curious if I add a console.log (and nothing else), e.g.:

const logStreamEntries = await infraLogStream.getStreamEntries();
console.log( logStreamEntries);
expect(logStreamEntries.length).to.be.greaterThan(0);

then there's no longer a length of 0, the items exist, and the test passes.

It's very weird, and very flaky 🙈

===============================================

Checklist

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

Kerry350 added 12 commits July 30, 2019 10:42
* Adds nested routing for logs
* Adds an index page to handle shared concerns
* Adds the Stream page at /logs/stream
* Introduces shared/settings page
* Adds shared/settings page as a tab in the Logs routes
* Removes previous source configuration flyout traces from Logs pages
* Implements use of EUI Panels
* Centers page content
* Add discard button to allow resetting the form
* Align Apply and Discard buttons to the right
* Align Loading button to the right
* Add EuiDescribedFormGroup for name panel
* Add EuiDescribedFormGroup for indices panel
* Add EuiDescribedFormGroup for fields panel
* Adds a ViewSourceConfigurationButton component that will route to the /settings page
* Replace all instances of "View Configuration" buttons that were opening the flyout with the new button
* Introduces an AppNavigation component
* Amends styling / handling of RoutedTabs to match SIEM implementation
* Adds new AppNavigation component to Infrastructure and Logs indexe pages
@Kerry350 Kerry350 added release_note:enhancement review Feature:Metrics UI Metrics UI feature Feature:Logs UI Logs UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services :infrastructure labels Jul 30, 2019
@Kerry350 Kerry350 requested a review from a team July 30, 2019 11:31
@Kerry350 Kerry350 self-assigned this Jul 30, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/infra-logs-ui

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@simianhacker simianhacker self-requested a review July 30, 2019 15:52
@jasonrhodes jasonrhodes removed the request for review from a team July 30, 2019 20:13
@Kerry350 Kerry350 requested a review from a team as a code owner July 31, 2019 11:53
* Ensure outline isn't cut off
* Ensure only the react-router instance is hit for performance
* Ensure links still have href attributes for things like "Open in a new tab" even if history.push ultimately navigates
@Kerry350
Copy link
Contributor Author

Kerry350 commented Aug 2, 2019

@Zacqary I've addressed your feedback so far:

The navigation is now more performant when switching, and the visual discrepencies are cleaned up.

navigation

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@Zacqary
Copy link
Contributor

Zacqary commented Aug 2, 2019

@Kerry350 Looks great! Are you able to take it a step further and prevent the logs stream from having to reload data when switching away from the Settings tab?

If it's very complicated to fix, that might not be the biggest issue. Users will probably be changing settings when they switch to the Settings tab which would require a stream reload anyway. Unless persisting the logs data when you switch to the Settings tab would also allow it to start loading new data before you even switch away from the tab?

@Zacqary
Copy link
Contributor

Zacqary commented Aug 2, 2019

Can we also get some kind of "Are you sure?" prompt if you make changes, then click the Stream tab instead of clicking Apply? Otherwise those changes are lost

@Kerry350
Copy link
Contributor Author

Kerry350 commented Aug 5, 2019

@Zacqary

Are you able to take it a step further and prevent the logs stream from having to reload data when switching away from the Settings tab?

If it's very complicated to fix, that might not be the biggest issue. Users will probably be changing settings when they switch to the Settings tab which would require a stream reload anyway. Unless persisting the logs data when you switch to the Settings tab would also allow it to start loading new data before you even switch away from the tab?

This would be a very involved and non-trivial change, and I doubt we want to focus time in that direction at the moment given the feature priorities.

I realise that comes with an air of negativity so I wanted to highlight architecturally why it's not a simple improvement:

  • Historically we don't use caching functionality. Log entries are served - data wise - by a combination of GraphQL and Redux. On the client GraphQL side we turn off caching with a fetchPolicy of no-cache, the reason for this is Apollo's caching functionality was found to have quirks / bugs. We don't cache within Redux for later consumption either. Adding on to that, we want to, very soon, remove GraphQL and Redux usage from the locations they still exist. So we don't, as it stands, have an easy way to instantly re-access data that was previously queried for display.

  • Every independent page (currently) handles it's data in an independent way. When the stream page mounts it fetches and displays it's data, the same happens for settings (+ inventory and metrics-explorer). They don't share anything in any way anymore, this was different with the flyout as it was rendered within every page. This loops back in to either having a form of caching mechanism, or hoisting some sort of data sharing mechanism to the index page (also not in place).

  • We don't actually persist last used query parameters when navigating between tabs, so when you go to /settings and then back to /stream, when the stream page re-mounts it will set itself back up with a new point in time etc.

If this was something we wanted to look at long term - now that we have more "pages" coming in and more navigation needs - I'd say we need to look at this as a whole team and agree upon an approach for the whole of infrastructure + logs. Which, I personally feel, wouldn't make sense until we've got everything moved over to the "simple HTTP API" approach. Even in that scenario, for caching, we can't make use of something simple like regular HTTP cache headers as we use POST.

@Kerry350
Copy link
Contributor Author

Kerry350 commented Aug 5, 2019

@Zacqary

Can we also get some kind of "Are you sure?" prompt if you make changes, then click the Stream tab instead of clicking Apply? Otherwise those changes are lost

This would be a nice addition, I'll look at this now 👍

@elasticmachine
Copy link
Contributor

💔 Build Failed

@Kerry350
Copy link
Contributor Author

Kerry350 commented Aug 5, 2019

@Zacqary I've added in a prompt for when changes are about to be lost. I've used the <Prompt /> component that ships directly with react-router. It works well, the only downside (well, that's up for debate) is it uses a native browser prompt, so it's not stylised at all. I don't think this is an issue, given how rarely it will probably be seen.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@Zacqary Zacqary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looks good to me! Great work!

@Kerry350
Copy link
Contributor Author

Kerry350 commented Aug 6, 2019

Awesome, thanks for the reviews.

Just a note to say I've got a small followup PR that will come with a fix for the breadcrumbs being incorrect sometimes. It wasn't broken by this work, it exists in Master, but I spotted it as part of the navigation work. But I don't want to block this big ol' chunk getting merged.

@Kerry350 Kerry350 merged commit ad111be into elastic:master Aug 6, 2019
Kerry350 added a commit to Kerry350/kibana that referenced this pull request Aug 7, 2019
…ze the main navigation (elastic#42243)

* Setup Logs routing for multiple pages

* Adds nested routing for logs
* Adds an index page to handle shared concerns
* Adds the Stream page at /logs/stream

* Introduce shared settings page

* Introduces shared/settings page
* Adds shared/settings page as a tab in the Logs routes
* Removes previous source configuration flyout traces from Logs pages

* Begin styling adjustments to settings page

* Implements use of EUI Panels
* Centers page content

* Add discard button

* Add discard button to allow resetting the form

* Fix button alignment

* Align Apply and Discard buttons to the right
* Align Loading button to the right

* Add EuiDescribedFormGroup for all form fields

* Add EuiDescribedFormGroup for name panel
* Add EuiDescribedFormGroup for indices panel
* Add EuiDescribedFormGroup for fields panel

* Remove all SourceConfigurationFlyout traces from the Infrastructure UI

* Add a ViewSourceConfigurationButton

* Adds a ViewSourceConfigurationButton component that will route to the /settings page
* Replace all instances of "View Configuration" buttons that were opening the flyout with the new button

* Enable settings tab amongst Infrastructure routes

* Change navigation to mimic SIEM

* Introduces an AppNavigation component
* Amends styling / handling of RoutedTabs to match SIEM implementation
* Adds new AppNavigation component to Infrastructure and Logs indexe pages

* Functional test amendments (WIP)

* Temporarily disable certain functional tests

* Remove unused imports

* Disable imports so build can pass

* Amend I18N errors

* I18N

* Automatically fix issues with i18n (node scripts/i18n_check --fix result)

* Functional tests

* Amend tests so they pass locally. Pending CI test.

* Amend RoutedTabs (without link focus style)

* Tweak RoutedTabs and AppNavigation for better performance / visuals

* Ensure outline isn't cut off
* Ensure only the react-router instance is hit for performance
* Ensure links still have href attributes for things like "Open in a new tab" even if history.push ultimately navigates

* Fix i18n usages

* node scripts/i18n_check --fix

* Amend functional test config (post Master merge fix)

* Remove unused function and fix unused import

* Add a Prompt to notify users when form changes will be lost

* Add aria-label to Button
Kerry350 added a commit that referenced this pull request Aug 8, 2019
…ze the main navigation (#42243) (#42887)

* Setup Logs routing for multiple pages

* Adds nested routing for logs
* Adds an index page to handle shared concerns
* Adds the Stream page at /logs/stream

* Introduce shared settings page

* Introduces shared/settings page
* Adds shared/settings page as a tab in the Logs routes
* Removes previous source configuration flyout traces from Logs pages

* Begin styling adjustments to settings page

* Implements use of EUI Panels
* Centers page content

* Add discard button

* Add discard button to allow resetting the form

* Fix button alignment

* Align Apply and Discard buttons to the right
* Align Loading button to the right

* Add EuiDescribedFormGroup for all form fields

* Add EuiDescribedFormGroup for name panel
* Add EuiDescribedFormGroup for indices panel
* Add EuiDescribedFormGroup for fields panel

* Remove all SourceConfigurationFlyout traces from the Infrastructure UI

* Add a ViewSourceConfigurationButton

* Adds a ViewSourceConfigurationButton component that will route to the /settings page
* Replace all instances of "View Configuration" buttons that were opening the flyout with the new button

* Enable settings tab amongst Infrastructure routes

* Change navigation to mimic SIEM

* Introduces an AppNavigation component
* Amends styling / handling of RoutedTabs to match SIEM implementation
* Adds new AppNavigation component to Infrastructure and Logs indexe pages

* Functional test amendments (WIP)

* Temporarily disable certain functional tests

* Remove unused imports

* Disable imports so build can pass

* Amend I18N errors

* I18N

* Automatically fix issues with i18n (node scripts/i18n_check --fix result)

* Functional tests

* Amend tests so they pass locally. Pending CI test.

* Amend RoutedTabs (without link focus style)

* Tweak RoutedTabs and AppNavigation for better performance / visuals

* Ensure outline isn't cut off
* Ensure only the react-router instance is hit for performance
* Ensure links still have href attributes for things like "Open in a new tab" even if history.push ultimately navigates

* Fix i18n usages

* node scripts/i18n_check --fix

* Amend functional test config (post Master merge fix)

* Remove unused function and fix unused import

* Add a Prompt to notify users when form changes will be lost

* Add aria-label to Button
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Logs UI Logs UI feature Feature:Metrics UI Metrics UI feature release_note:enhancement review Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants