-
Notifications
You must be signed in to change notification settings - Fork 843
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
[EuiBreadcrumbs] a11y polish #4763
Conversation
Preview documentation changes for this PR: https://eui.elastic.co/pr_4763/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4763/ |
jenkins test this |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4763/ |
@myasonik So the big thing that I'm seeing is that, in Amsterdam, the EuiHeaderBreadcrumbs are a wrapper around EuiBreadcrumbs with some specific styling overrides. This is now rendering quite wrong. 😬 I think mainly due to moving the separator into CSS. |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4763/ |
src/components/header/header_breadcrumbs/_header_breadcrumbs.scss
Outdated
Show resolved
Hide resolved
Preview documentation changes for this PR: https://eui.elastic.co/pr_4763/ |
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! Tested in all the browsers using the preview link
Preview documentation changes for this PR: https://eui.elastic.co/pr_4763/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4763/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4763/ |
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.
Preview documentation changes for this PR: https://eui.elastic.co/pr_4763/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4763/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4763/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4763/ |
…c#4977) * handle 0 count * CL
Co-authored-by: cchaos <caroline.horn@elastic.co>
* [EuiLink] Increased font weight to match buttons - Increased text underline weight on focus - Removed low-contrast focus background - Added `:visited` and `:target` styling (All mainly affecting Amsterdam) * Re-use mixin in text styles
WEll that was a terrible upstream merged... Sorry, will fix |
# Conflicts: # src/components/link/_link.scss # src/themes/eui-amsterdam/global_styling/mixins/_link.scss
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 knew there was going to be some issue with my EuiLink PR and increased font-weights, so I went ahead and fixed those.
Let's get this sucker merged 👍 😆
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
Preview documentation changes for this PR: https://eui.elastic.co/pr_4763/ |
1 similar comment
Preview documentation changes for this PR: https://eui.elastic.co/pr_4763/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4763/ |
Summary
Closes #3624
Biggest change is the DOM structure:
- From a
<nav>
with<a>
,<button>
and<span>
children- To a
<nav>
with an<ol>
, where each<li>
has a<a>
,<button>
or<span>
child- Separators are now rendered 100% through CSS instead of JSX+CSS
Some tiny visual changed with with spacing tightened up and relying on more native CSS logic
Introduces a new prop for Breadcrumb:
aria-current
Allows consumers to override the default value of
page
, only valid on the last itemChecklist
[ ] Props have proper autodocs and playground toggles[ ] Added documentation[ ] Checked Code Sandbox works for the any docs examples