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

New nav #64018

Merged
merged 54 commits into from
May 6, 2020
Merged

New nav #64018

merged 54 commits into from
May 6, 2020

Conversation

myasonik
Copy link
Contributor

@myasonik myasonik commented Apr 20, 2020

Fixes #56232
Also fixes: #59039 by chance

Summary

New collapsible nav! (There's an advanced setting to bring the old nav.)

See our plan for test coverage: #65369

Screen Shot 2020-04-22 at 19 00 31

Kapture 2020-04-22 at 19 10 48

Initial build checklist


Feedback to fix

  • Change 'Visualization Library' to 'Visualize' (see this comment)
  • Move Recently viewed above Kibana to keep with old nav placement (ultimately this will move to Global Search)
  • When docked, hide menu button in header (see showButtonIfDocked prop)
  • Bottom bar width calculation (see first screenshot here)
  • When not docked, hide menu after any nav click (see first bullet here)
  • Make sure all apps use full screen height now that there is not an always visible nav (see example here) Layout breaks have been observed in:
    • Visualize
    • Lens
    • Graph
  • Correct header height variable; extra height results in extra top menu padding (see second bullet in this comment) and can force a vertical scroll (see Maps example in fifth screenshot here) [Note: The real fix is in EUI but putting an override into Kibana for the time being to not rush an EUI update right before feature freeze]
  • Always hide menu after a nav link click (see first bullet in this comment)
  • Force nav and its overlay mask to be above other flyouts (see screenshots 3 and 6 in this comment)
  • Adjust popover direction for Dashboard items (see bullet items 3 and 4 in this comment)
  • Fix full screen display of Dashboard (see bullet 1 in this comment)
  • Fix full screen display of an individual Dashboard panel (see bullet 1 in this comment)
  • If related to nav change, fix Space selection layout (see this comment) Layout of spaces picker is same as it is on master

Feedback to watch

  • Is the Home link atop the nav useful or simply redundant?
    We should keep in mind that pinning will soon also fill this space, so the Home link keeps this section always visible even when no pins exists. That could be accomplished with a title, for example, however there is some convenience with regards to keyboard navigation by keeping it here. Let's keep it for now and monitor user feedback.
  • Is the overlay mask necessary?
    It blocks all other screen interaction which may or may not be desirable. For cases where another flyout is open from the right side, we can cover those with the nav overlay which may reduce some confusion. It also forces an interaction with regards to navigation before all else. This may make it easier to manager the open/close state of the nav. Let's keep it as-is for now and monitor user feedback.
  • Is the nav width unnecessarily wide?
    As it stands, none of the links within the nav exceed even half of the width which is currently set at 320px. User may perceive this is a waste of screen space as they did with the previous nav design which was 240px. There is an opposing opinion that the new visual design/prominence may warrant the extra size. Additionally, we may consider stepped widths for varying screen sizes. Let's keep it as-is for now and monitor user feedback.
  • How much confusion is generated by naming the first section Kibana?
    Product has taken the lead on this topic and is consulting internal stakeholders. @alexfrancoeur will communicate the final decision.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@ryankeairns

This comment has been minimized.

@myasonik myasonik force-pushed the new-nav branch 2 times, most recently from 8d2581e to 25aec24 Compare April 23, 2020 00:07
@myasonik
Copy link
Contributor Author

myasonik commented Apr 23, 2020

Promoting this to not a draft to encourage more eyes on it early because of how big of a change this is. Sorry for the pings y'all!

I believe this to be functionally complete.

Now is a great time to:

  • Review the code
  • Review the functionality, particularly:
    • Making sure I didn't break anything in the old nav
    • Making sure the order and name changes work/are expected

Known a11y issues:

  1. Locking the nav the first time will force focus to be placed back at the start of the page. This was a tradeoff to put the nav in the correct tab order which I think is a worthwhile trade. (A onetime annoyance of focus being moved vs a persistent annoyance of the nav being in the wrong spot.) The problem can likely be fixed by tuning how often the nav rerenders which will have the added benefit of less rerendering (read: faster).
  2. When closing the nav, focus isn't consistently sent to the same location. (Gets sent to the button if the nav was not docked previously, gets sent somewhere onto the page if the nav was previously docked.) EUI needs to be updated to accept a ref on header buttons to fix this.

@myasonik myasonik marked this pull request as ready for review April 23, 2020 00:08
@myasonik myasonik requested a review from a team April 23, 2020 00:08
@myasonik myasonik requested review from a team as code owners April 23, 2020 00:08
@lizozom
Copy link
Contributor

lizozom commented Apr 23, 2020

🔥 🔥 🔥 🔥 Dock navigation option 🔥 🔥 🔥 🔥

@ryankeairns

This comment has been minimized.

@ryankeairns

This comment has been minimized.

@snide

This comment has been minimized.

@snide
Copy link
Contributor

snide commented Apr 23, 2020

@lindseypoli Given the rework of our security products, it might make sense to start breaking down the individual views under Security into separate nav links. Right now it's just SIEM, but my guess is we can probably link directly to some of these spots (signals, cases...etc) from the nav directly.

@myasonik myasonik requested a review from a team as a code owner April 23, 2020 22:59
@myasonik myasonik removed Feature:Drilldowns Embeddable panel Drilldowns Feature:Embedding Embedding content via iFrame Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels May 5, 2020
Copy link
Contributor

@shahzad31 shahzad31 left a comment

Choose a reason for hiding this comment

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

Uptime changes LGTM !

@botelastic botelastic bot added Feature:Drilldowns Embeddable panel Drilldowns Feature:Embedding Embedding content via iFrame Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels May 5, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@ryankeairns
Copy link
Contributor

In response to the feedback regarding additional unit tests, coupled with feedback from Product regarding delivery of this feature in 7.8, we'll be making a concerted effort to continue bulking up test coverage post-merge. We discussed this as a team and understand that this procedure (continuing to write tests post-FF) is an exception to the norm, but feel it is the best approach for this particular case.

A separate issue has been created here to track that additional work around testing: #65369

There were existing navigation tests in place prior to this PR (which have been updated here), but more work ought to be done. Semi-relatedly, there is an existing deficiency for tests concerning the header which we would also like to improve.

@ryankeairns ryankeairns mentioned this pull request May 6, 2020
7 tasks
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@myasonik myasonik merged commit 35e1027 into elastic:master May 6, 2020
@myasonik myasonik deleted the new-nav branch May 6, 2020 04:14
@myasonik myasonik mentioned this pull request May 6, 2020
myasonik pushed a commit that referenced this pull request May 6, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request May 6, 2020
* master: (72 commits)
  add tsvb tests to Firefox suite (elastic#65425)
  Fix flaky ServerMetricsCollector integration test (elastic#65420)
  [APM] Custom links section inside the Actions menu is showing outside of the menu (elastic#65428)
  [ML] Adds docs_per_second to transform edit form. (elastic#65365)
  update apm index pattern (elastic#65424)
  add direct build command (elastic#65431)
  [ML] Adding daily_model_snapshot_retention_after_days to types and schemas (elastic#65417)
  [chore] Improve request cancelation handling in vis embeddable (elastic#65057)
  [Alerting] migrates acceptance and functional test fixtures to KP (elastic#64888)
  [ML] Fixes reordering in view by selection when overall cell selected (elastic#65290)
  Additional branding updates (elastic#64712)
  Remove redundant formatting of percentage column (elastic#64948)
  [SIEM][CASE] Configuration pages UI redesign (elastic#65355)
  New nav (elastic#64018)
  [Ingest pipelines] Address copy feedback (elastic#65175)
  bug fixing (elastic#65387)
  skip whole suite blocking snapshots (elastic#65377)
  add related event generation to ancestor nodes (fixes a bug) (elastic#64950)
  [Canvas] move files from legacy/plugins to plugins (elastic#65283)
  [SIEM] template timeline UI (elastic#64439)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Drilldowns Embeddable panel Drilldowns Feature:Embedding Embedding content via iFrame release highlight release_note:enhancement Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.8.0 v8.0.0
Projects
None yet