-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Refine the visual design, interaction, and accessibility of the global navigation #57455
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
Conversation
|
Nice |
ab1c359 to
d7393cb
Compare
bbovenzi
left a comment
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.
Love it!
|
Love it ❤️ |
guan404ming
left a comment
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.
Looks really great!
|
This looks great. Love it |
phanikumv
left a comment
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.
Looks great!
sunank200
left a comment
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.
Amazing
pierrejeambrun
left a comment
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, thanks!
58a26a6 to
939b813
Compare
amoghrajesh
left a comment
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.
Looks nice!
939b813 to
7504c6a
Compare
|
Very cool... just one minor nit: As you had the example of german translation above with overflowing label... this is now hard to read. Could we use at least a line break to have a text in two lines? Or is it always cropping first line? @TJaniF might we need / shall we adjust German translation for the change? WDYT |
@jscheffl It's intentionally always cropping to a single line. This is how we can enforce a consistent presentation across the navigation items, regardless of what value is entered in the translation file. Given there are only a handful of these nav items and they are persistently visible in the application, the icons really act as the primary identifiers since people can process symbols quicker than words and the labels are secondary. FWIW, In your example, the label is
|

Refines the visual presentation of the global navigation
While some of the changes in this PR have a subjective nature to them, I believe the sum makes for an objectively better user experience.
Improves nav item accessibility
Observe how each nav item was receiving focus twice each time I pressed tab on my keyboard:
This was due to undesirable markup nesting with anchors wrapping buttons. This was resolved so that each nav item is only an anchor or only button (where appropriate), utilizing Chakra's
asChildprop to accomplish this aswell as some additional refactoring of theNavButtoncomponent.Testing
Added proper overflow handling and title tags for languages that might have overly long labels on them:
I confirmed no regressions with RTL languages:
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.