-
Notifications
You must be signed in to change notification settings - Fork 842
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
Add header links, replace header breadcrumbs, fix accessibility issues around them #844
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.
LGTM, love those deletions!
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. Nice work on the accessibility stuff! Thanks for doing that. Just had a few suggestions.
}], | ||
text: ( | ||
<p> | ||
Kibana should and will eventually always use the breadcrumb style of header. |
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.
If you want, you can future-proof this copy by leaving this bit about Kibana out (I'm also not sure if this guidance will have an effect on people reading the docs, since once we implement this header in Kibana it's pretty much a done deal).
If you’re using EUI in a one-off site or page, you can use <EuiCode>EuiHeaderLinks</EuiCode>, <EuiCode>EuiHeaderLink</EuiCode>s instead of breadcrumbs.
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.
Makes sense.
@@ -74,7 +74,11 @@ $buttonTypes: ( | |||
} | |||
|
|||
&:focus { | |||
background-color: transparentize($color, .9); | |||
@if ($name == 'text') { |
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.
Is this a bug-fix? Is it worth mentioning in the change log?
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 a pretty minor visual change. Didn't think it was worth mentioning.
@@ -161,6 +163,15 @@ export class EuiPopover extends Component { | |||
ariaLive = 'assertive'; | |||
} | |||
|
|||
let focusTrapScreenReaderText; |
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 love this idea but VoiceOver on OS X wasn't picking it up when I was testing the header popovers. I have VO verbosity set to high -- did you do anything special to your settings or maybe you're using a different screen reader?
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'll check it out again. Was ownFocus
on? For example, this wouldn't work in the combo box cuz i think it's set to off there. The default popover example has it off as well. Try it in the header.
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 was testing it with the header popovers, e.g. the keypad menu and the user menu. No dice. The first focusable elements with those popovers automatically took focus and VO read those aloud.
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.
Here's what I get.
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.
The first one has ownFocus off, the second one has it on. Works the same for me in the header. Since it's an alert, it should read it first, before the focus.
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.
Oh really weird, I just tried it again and this time it worked. Must have been a VO glitch. Anyway, it sounds & looks great!
Also made the following modifications to existing comps: - `EuiHeaderLogo` now accepts text (children) as well - `EuiHeaderSectionItem` can have “none” as the border but still defaults to “left”
…r` for dark theme
Replaces #804
Fixes #801
Fixes #727
Fixes #726
Summary
EuiHeaderBreadcrumb
components withEuiBreadcrumbs
EuiHeaderLink
for more traditional navigation uses (@thomasneirynck needs this).Later todo