-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Uptime] Move uptime to new solution nav #100905
Conversation
Pinging @elastic/uptime (Team:uptime) |
Note that title are wrapped in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's really exciting seeing this design come to life!
Looking at the issue for this it seems we also wanted to add a new breadcrumb for the overview page. I think this difference is largely semantic, as both the Uptime
and Monitoring overview
breadcrumb will be disabled for that page.
Also, I am noticing that the Uptime
breadcrumb should be disabled when we're on the overview page and today it isn't, so it links back to the current page. Sometimes this functionality is intentional for things like clearing filters, but our page doesn't do that either. I'd be fine fixing that as part of this change or creating a follow-up issue.
Additionally, I noticed the focus moves to the newly-loaded page after navigating via the keyboard. I'd imagine this is controlled more by the nav component than the revisions in this PR but I did find it jarring. This new nav UI frames Observability as a more unified app and I didn't plan on focus moving from the universal nav after choosing a view. Curious to hear @dominiqueclarke's feedback on this.
@justinkambic i have added the breadcrumb for Monitoring overview, just to be clear, i have kept uptime clickable, since i think it's natural to do that. I didn't understand your focus shifting question though. |
💚 Build SucceededMetrics [docs]Async chunks
Page load bundle
Unknown metric groupsReferences to deprecated APIs
History
To update your PR or re-run it, just comment with: cc @shahzad31 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good. Regarding the focus issue I was talking about, I've included a new GIF below:
When navigating this UI with my keyboard only, it's really hard for me to quickly switch between views and it feels as though it should be much easier. I'm not sure if this is an issue with the underlying nav API or with our app but it could improve.
@justinkambic yes you are right, the tab order is weird. It's controlled by shared component. I will log an issue with EUI team to take a look at this. |
@weltenwort @cchaos any ideas, how this keyboard nav can be improved? |
That's what I figured, I don't see any need to hold this PR based on that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
* Expose options to customize the route matching * Add more comments * move uptime to new solution nav * push * update test * add an extra breadcrumb Co-authored-by: Felix Stürmer <stuermer@weltenwort.de>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
* Expose options to customize the route matching * Add more comments * move uptime to new solution nav * push * update test * add an extra breadcrumb Co-authored-by: Felix Stürmer <stuermer@weltenwort.de> Co-authored-by: Shahzad <shahzad.muhammad@elastic.co> Co-authored-by: Felix Stürmer <stuermer@weltenwort.de>
Hey @shahzad31 , I know you just merged this, but I'm seeing something odd in the layout. It looks like there's padding around the whole template causing the side nav and panels not to attach to the window edges. Is this an artifact of the placement of the observability specific template or where the Kibana template is placed? As for the focus, neither the Kibana or EUI page templates do any forcing of focus states. |
@cchaos that's a good catch, i have removed that padding in this PR #101005 |
Summary
Fix: #99901
Move uptime to new solutions nav
The following considerations have been addressed
Know Issues: