-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Re-style and re-order top menu buttons #79206
Conversation
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.
😍 I'm really diggin the consistent placement and look to the Save/Edit buttons. That'll go a long way for users.
The only one that fails that a bit is the dashboard one, which even the wording of that one "Add new" is confusing because of it's location in the menu bar I'd assume the "New" in that context is a new dashboard. But I know this was a decision made a while back, and by the looks of the "Looking ahead" mocks, that button might move when there's more options.
Thanks for pulling this one together @ryankeairns !
Woo! |
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.
This design is looking great - my only problem with the last one was that there wasn't enough indication of which item the icon belonged to, and this addresses that. Moving the emphasized item to the right, also works quite well here.
I tested on chrome, and did run into a super tiny issue, where the hover state would slightly cut off the top outline of the button:
Otherwise though, this is really solid.
Thanks @ThomThomson |
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.
Knowing that the above tiny issue will be fixed, everything LGTM
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.
Good catch @nreese . The order has been fixed and, as with the others, only the primary call-to-action (i.e. the button) will show an icon. In this case, when Save and Return is visible, then the icon will not show for Save As. Default stateFrom Dashboard |
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
code review, tested in chrome
💚 Build SucceededMetrics [docs]async chunks size
page load bundle size
History
To update your PR or re-run it, just comment with: |
* Re-style and re-order top menu buttons * Update snapshot due to removed fill prop * Fix link order for Maps
Relates to #72331 (comment)
Summary
With the new stacked header design, 'top menus' (in the legacy Kibana apps) were moved from the app level to the chrome/header level. For some (author included :) ), this has resulted in an undesirable visual design/UX outcome for apps that used filled buttons in their menu. For example:
Competing buttons in Dashboard
Competing buttons in Lens
Proposed 7.10 solution introduced by this PR
checkInCircleFilled
andplusInCircleFilled
)Affected apps (Dashboard, Lens, Visualize, Maps)
Dashboard (edit mode)
Dashboard (view mode)
Lens
Visualize
Visualize (from Dashboard)
Maps
Looking ahead
The recommended approach gets even more 'bold' when viewed in the Amsterdam theme. With the changes proposed by this PR, it would look as follows when the theme is enabled. Additionally, the plan is for this new theme to become the default later in the 7.x series.
In addition to the top menu, teams may begin exploring additional UI enhancements if they desire to further emphasize core actions. For example, the Dashboard team is exploring adding a toolbar within their UI (for this and other reasons):
Checklist
Delete any items that are not applicable to this PR.