Repository navigation
Fix accessibility issues with saved object finder - #13152
Conversation
and use new kui styles
cjcenizal
left a comment
There was a problem hiding this comment.
I love that you swapped in the kuiTabs component -- thanks for doing that! I have just one small suggestion.
| </li> | ||
| </ul> | ||
| <div class="kuiTabs"> | ||
| <div |
There was a problem hiding this comment.
These can be <button> elements, and then we won't need kbn-accessible-click any more. That directive is meant to be used as a last resort, if you can't use a <button> or <a> with an href.
|
Also can we leave #12205 open? I think these will need to be long-lived issues to help us track overall progress and write up our 508 accessibility report. |
|
I don't think kuiTabs are darkThemed yet so this will look wrong on a dark theme dashboard. |
|
Oops @trevan is right. I guess we'll have to forgo the tabs for now, unless you want to update the UI Framework @stacey-gammon. |
|
I'm probably going about this all wrong, but some experimentation yields this for a dark theme variation: |
cjcenizal
left a comment
There was a problem hiding this comment.
🛠 It's so cool to see this component get updated. Thanks for doing this. Just had two suggestions!
| $tabColor--darkTheme: #cecece; | ||
| $tabBackgroundColor--darkTheme: #333333; | ||
| $tabHoverBackgroundColor--darkTheme: #777777; | ||
| $tabBorderColor--darkTheme: #777777; |
There was a problem hiding this comment.
This is fantastic! I'd just suggest looking for global color variables (in https://github.com/elastic/kibana/blob/master/ui_framework/components/_variables.scss#L47), to which you can assign these variables. For example:
$tabColor--darkTheme: $globalTextColor--darkTheme;
$tabBackgroundColor--darkTheme: $globalBackgroundColor--darkTheme;
// etc...This way all of the components will derive the same color palette from a shared source of truth. If there are any colors which don't exist in the _variables.scss file, then feel free to just assign the hex value to a component-specific variable in this file.
|
|
||
| @include darkTheme { | ||
| background-color: $tabBackgroundColor--darkTheme; | ||
| border-color: $tabBorderColor--darkTheme; |
There was a problem hiding this comment.
I think we'll also need to add a color property here to make it match the link's dark theme style (https://github.com/elastic/kibana/blob/master/ui_framework/components/_mixins.scss#L25).
color: $globalLinkColor--darkTheme;Similar changes will need to be made to the hover and focus states.
There was a problem hiding this comment.
I added colors for the hover states, but the other states I left as is to follow suit with the light theme, which doesn't treat those tabs when they are active as links.
How it looks now:
There was a problem hiding this comment.
Oh OK cool, I see. I took a look in the browser and it looks like the text color has poor color contrast against the background:
I think we should address this color contrast issue. How about using a different variable -- can we add color: $tabColor--darkTheme;? This will get it to look like this:
|
jenkins, test this |
cjcenizal
left a comment
There was a problem hiding this comment.
Had one small suggestion. Also, can we add an example for the Tabs dark theme to the documentation site?
<GuideDemo isDarkTheme={true}>
<Tabs />
</GuideDemo>|
|
||
| @include darkTheme { | ||
| background-color: $tabBackgroundColor--darkTheme; | ||
| border-color: $tabBorderColor--darkTheme; |
There was a problem hiding this comment.
Oh OK cool, I see. I took a look in the browser and it looks like the text color has poor color contrast against the background:
I think we should address this color contrast issue. How about using a different variable -- can we add color: $tabColor--darkTheme;? This will get it to look like this:
…ccessibility-object-finder
|
Failed on: Hope this isn't an unstable test, those were supposed to be fixed. :( If it passes a second time, I'll file a bug for unstable test. Since it seems to have failed on the merge commit and not the prior commit, that's what I suspect. jenkins, test this |
|
Should be ready to go for a final look @cjcenizal. Test failure was flaky and I have a separate PR out that should address it. |
* Fix accessibility issues with saved object finder and use new kui styles * Dark theme-icy kuiTabs * Refer to existing dark theme color variables. Use dark theme hover link color. * use button instead of div element so no need for kbn-accessible-click * Add dark theme tab variety to ui framework site, lighten color of background tabs
* Fix accessibility issues with saved object finder and use new kui styles * Dark theme-icy kuiTabs * Refer to existing dark theme color variables. Use dark theme hover link color. * use button instead of div element so no need for kbn-accessible-click * Add dark theme tab variety to ui framework site, lighten color of background tabs
* Fix accessibility issues with saved object finder and use new kui styles * Dark theme-icy kuiTabs * Refer to existing dark theme color variables. Use dark theme hover link color. * use button instead of div element so no need for kbn-accessible-click * Add dark theme tab variety to ui framework site, lighten color of background tabs
* Fix accessibility issues with saved object finder and use new kui styles * Dark theme-icy kuiTabs * Refer to existing dark theme color variables. Use dark theme hover link color. * use button instead of div element so no need for kbn-accessible-click * Add dark theme tab variety to ui framework site, lighten color of background tabs
* Fix accessibility issues with saved object finder and use new kui styles * Dark theme-icy kuiTabs * Refer to existing dark theme color variables. Use dark theme hover link color. * use button instead of div element so no need for kbn-accessible-click * Add dark theme tab variety to ui framework site, lighten color of background tabs
and use new kui styles
Fixes #12484
Fixes #12485
Fixes #12483