-
Notifications
You must be signed in to change notification settings - Fork 839
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
[Emotion] Convert EuiTreeView #7513
Merged
Merged
Changes from 16 commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
e8e7f12
Set up style file + theme
cee-chen d1b60ab
🔥 Remove unnecessary text/div wrapper
cee-chen a3339ea
Convert `.euiTreeView__node` styles
cee-chen 1c2ae7d
Remove unnecessary .euiTreeView__nodeLabel CSS
cee-chen ad39f45
Convert `.euiTreeView__nodeInner`
cee-chen 4f5d0e7
Fix misaligned vertical icons
cee-chen 5645b52
Remove unnecessary horizontal margins in favor of `gap` property
cee-chen f6965df
Convert placeholder + restore arrow offset
cee-chen 9708ca9
Class/map cleanup
cee-chen e6e2b9a
[misc] syntax cleanup
cee-chen 542f014
Delete Sass files
cee-chen 79f8a71
Split out `EuiTreeViewItem` subcomponent
cee-chen 80dcb13
Tests
cee-chen 78b2980
Add Storybook + fix style bugs caught as a result
cee-chen afda048
[docs] Fix playground
cee-chen 1845fbf
changelog
cee-chen 6b0cc92
[PR feedback] aria-expanded
cee-chen ba29558
Voiceover SR behavior
cee-chen File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| **CSS-in-JS conversions** | ||
|
|
||
| - Converted `EuiTreeView` to Emotion. Updates as part of the conversion: | ||
| - Removed `.euiTreeView__wrapper` div node | ||
| - Enforced consistent `icon` size based on `display` size |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License | ||
| * 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
| * in compliance with, at your election, the Elastic License 2.0 or the Server | ||
| * Side Public License, v 1. | ||
| */ | ||
|
|
||
| import { css } from '@emotion/react'; | ||
|
|
||
| import { UseEuiTheme, transparentize } from '../../services'; | ||
| import { euiFocusRing, logicalCSS, mathWithUnits } from '../../global_styling'; | ||
|
|
||
| export const euiTreeViewItemStyles = (euiThemeContext: UseEuiTheme) => { | ||
| const { euiTheme } = euiThemeContext; | ||
|
|
||
| const defaultSize = euiTheme.size.xl; | ||
| const compressedSize = euiTheme.size.l; | ||
|
|
||
| return { | ||
| li: { | ||
| euiTreeView__node: css``, | ||
| default: css` | ||
| ${logicalCSS('max-height', defaultSize)} | ||
| line-height: ${defaultSize}; | ||
| `, | ||
| compressed: css` | ||
| ${logicalCSS('max-height', compressedSize)} | ||
| line-height: ${compressedSize}; | ||
| `, | ||
| expanded: css` | ||
| ${logicalCSS('max-height', '100vh')} | ||
| `, | ||
| }, | ||
|
|
||
| button: { | ||
| euiTreeView__nodeInner: css` | ||
| ${logicalCSS('width', '100%')} | ||
| ${logicalCSS('padding-left', euiTheme.size.s)} | ||
| ${logicalCSS('padding-right', euiTheme.size.xxs)} | ||
| display: flex; | ||
| align-items: center; | ||
|
|
||
| &:focus { | ||
| ${euiFocusRing(euiThemeContext, 'inset')} | ||
| } | ||
|
|
||
| &:hover, | ||
| &:active, | ||
| &:focus { | ||
| background-color: ${transparentize( | ||
| euiTheme.colors.text, | ||
| euiTheme.focus.transparency | ||
| )}; | ||
| } | ||
| `, | ||
| default: css` | ||
| ${logicalCSS('height', defaultSize)} | ||
| gap: ${euiTheme.size.s}; | ||
| border-radius: ${euiTheme.border.radius.medium}; | ||
| `, | ||
| compressed: css` | ||
| ${logicalCSS('height', compressedSize)} | ||
| gap: ${euiTheme.size.xs}; | ||
| border-radius: ${euiTheme.border.radius.small}; | ||
| `, | ||
| }, | ||
|
|
||
| icon: { | ||
| euiTreeView__iconWrapper: css` | ||
| flex-shrink: 0; | ||
| line-height: 0; /* Vertically centers the icon */ | ||
|
|
||
| /* Handle smaller icons in compressed mode */ | ||
| & > * { | ||
| ${logicalCSS('max-width', '100%')} | ||
| } | ||
|
|
||
| & > .euiToken { | ||
| ${logicalCSS('max-height', '100%')} | ||
| ${logicalCSS('height', 'auto')} | ||
|
|
||
| svg { | ||
| ${logicalCSS('width', '100%')} | ||
| } | ||
| } | ||
| `, | ||
| default: css` | ||
| ${logicalCSS( | ||
| 'width', | ||
| mathWithUnits(defaultSize, (x) => x / 2) | ||
| )} | ||
| `, | ||
| compressed: css` | ||
| ${logicalCSS( | ||
| 'width', | ||
| mathWithUnits(compressedSize, (x) => x / 2) | ||
| )} | ||
| `, | ||
| }, | ||
| }; | ||
| }; |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@1Copenut I just wanted to let you know that I made the opinionated decision to remove these
aria-labels from nestedEuiTreeViews. Could you compare staging vs. prod and let me know how that feels a11y-wise?I made the decision to remove the labels because I wasn't sure they were necessary, and WCAG's example doesn't appear to have them. WCAG uses very different
roles instead, which I briefly experimented with trying to implement, but it broke a bunch of stuff so I opted to not do it as part of the Emotion conversion.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.
TBH I like this implementation better than the W3C version,
at least for VoiceOverfor all three screen readers. The ARIA W3C is using structures the tree as a table with collapsed cells that can be expanded. That's a valid approach, but I think of a tree view more as nested lists, as you've got it structured. It was more clear to me being able to traverse through nested lists in all three screen readers.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.
Gotcha - just to confirm, were you okay with removing the aria labels? Do you think it's an improvement or degradation over production behavior?
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 went back and listened to the staging version and production again and felt there wasn't enough evidence to warrant keeping the
aria-labelsfor each list. This tree view is structured as a navigational element, would likely have headings around it and/or be in a<nav>landmark, and felt a little more natural hearing List, number of items being read out. Button names are a big key; if users have a good context when they expand a button, that's near-term memory for what they're likely to find inside that expanded button.After some more digging, Scott O'Hara comes through in the clutch for a second time on this PR:
https://www.scottohara.me/blog/2020/05/02/labelled-lists.html#naming-lists-situationally-helpful
tl;drAccessible labels on UL/OL make sense sometimes if there's a lot of lists that could be confused for one another. The use case for this component has me leaning toward A-OK to remove them in favor of native semantics.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.
Fantastic! Thank you so much Trevor!