-
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
[Emotion] Convert EuiTreeView #7513
Conversation
- just use text CSS instead - there's a ton of extra styles that come attached with `EuiText` that we really don't need here
- can't delete the `--expanded` CSS, Kibana has hooks into it
+ opinionated tweak: change border-radius to be smaller on compressed size + add a minor padding-right for extra text-truncation comfort + remove unnecessary CSS, `flex-direction` is already row by default, + no idea what that `text-align-last` is doing
+ remove unnecessary negative margins in favor of just letting flex align-center and `line-height: 0` do its thing
- prefer using a a placeholder span since arrows & tokens are the same size, + this lets gap work seamlessly
- remove className map - remove now-unnecessary `--withArrows` modifier (no usages in Kibana) + fix classNames syntax to match other components
+ remove `aria-label` behavior - this isn't actually useful information for screen readers per https://www.w3.org/WAI/ARIA/apg/patterns/treeview
- need `flex-shrink` on icon wrappers - enforce icon width/heights appropriately for consumers that switch between default and compressed styles
75a13eb
to
afda048
Compare
<ul | ||
aria-label="Item One child of aria-label" | ||
class="euiTreeView" |
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-label
s from nested EuiTreeView
s. 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 role
s 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 VoiceOver for 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-labels
for 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;dr
Accessible 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!
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 found one switch in the Storybook demo that I had a question about and a comment about the aria-expanded
attribute. LMK if you want to discuss and when you're ready for me to re-test.
9c2a84d
to
6b0cc92
Compare
className={classes} | ||
id={!this.isNested ? this.state.treeID : undefined} | ||
aria-describedby={!this.isNested ? instructionsId : undefined} | ||
{...rest} |
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.
@cee-chen I was re-testing with VoiceOver and discovered removing the containing div[role="group"]
left us with an unstructured list of items. Adding a role="list"
to the UL really shaped up the experience and shouldn't affect NVDA or JAWS because they retain the intrinsic understanding of UL being a list.
{...rest} | |
role="list" | |
{...rest} |
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.
Wait are you telling me VoiceOver doesn't automatically give ul
s a role="list"
??? 😕 what role does it get by default then?
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.
Well, none actually. Apple decided to remove the native role semantics when CSS was used to set list-style: none;
https://www.scottohara.me/blog/2019/01/12/lists-and-safari.html
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.
omg
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.
This required adding a lint rule skip (😭) and IMO is confusing enough to deserve its own inline comment as well. Thanks so much for the extra info Trevor! Today I (sadly) learned!
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.
Good call Cee. At some point Apple may fix this oversight and having the context inline will be helpful.
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 left a one-line suggestion to make VoiceOver + Safari honor the lists' primary role, but that doesn't need a re-test or reapproval. This is a clean implementation of a tree view and was easy to traverse and reason about. Thanks Cee!
8a98ba2
to
dda6306
Compare
Co-authored-by: Trevor Pierce <1Copenut@users.noreply.github.com>
dda6306
to
ba29558
Compare
Preview staging links for this PR:
|
💚 Build Succeeded
History
|
`v93.0.0` ⏩ `v93.1.1` --- ## [`v93.1.1`](https://github.com/elastic/eui/releases/v93.2.0) **This is a patch release primarily intended for use by Kibana.** - Added top-level `EuiTreeView.Item` export ([#7526](elastic/eui#7526)) ## [`v93.1.0`](https://github.com/elastic/eui/releases/v93.1.0) - Added `index` glyph to `EuiIcon` ([#7498](elastic/eui#7498)) - Updated `EuiHighlight` to accept an array of `search` strings, which allows highlighting multiple, separate words within its children. This new type and behavior *only* works if `highlightAll` is also set to true. ([#7496](elastic/eui#7496)) - Updated `EuiContextMenu` with a new `panels.items.renderItem` property, which allows rendering completely custom items next to standard `EuiContextMenuItem` objects ([#7510](elastic/eui#7510)) - `EuiSuperDatePicker` updates: - Updated `EuiSuperDatePicker` with a new `refreshIntervalUnits` prop. Passing this prop allows controlling and overriding the default unit rounding behavior. ([#7501](elastic/eui#7501)) - Updated `EuiAutoRefresh` and `EuiRefreshInterval` with a new `intervalUnits` prop. Passing this prop allows controlling and overriding the default unit rounding behavior. ([#7501](elastic/eui#7501)) - Updated `onRefreshChange` to pass back a new `intervalUnits` key that contains the current interval unit format (seconds, minutes, or hours). ([#7501](elastic/eui#7501)) - Updated `EuiSuperDatePicker` with a new `canRoundRelativeUnits` prop, which defaults to true (current behavior). To preserve displaying the unit that users select for relative time, set this to false. ([#7502](elastic/eui#7502)) - Updated `EuiSuperDatePicker` with a new `refreshMinInterval` prop, which accepts a minimum number in milliseconds ([#7516](elastic/eui#7516)) - Updated `EuiAutoRefresh` and `EuiRefreshInterval` with a new `minInterval` prop, which accepts a minimum number in milliseconds ([#7516](elastic/eui#7516)) **Bug fixes** - Fixed `EuiHighlight` to not parse `search` strings as regexes ([#7496](elastic/eui#7496)) - Fixed `EuiSuperDatePicker` submit bug when used within `<form>` elements ([#7504](elastic/eui#7504)) - Fixed an `EuiTreeView` bug where `aria-expanded` was being applied to items without expandable children ([#7513](elastic/eui#7513)) **CSS-in-JS conversions** - Converted `EuiTreeView` to Emotion. Updates as part of the conversion: ([#7513](elastic/eui#7513)) - Removed `.euiTreeView__wrapper` div node - Enforced consistent `icon` size based on `display` size
`v93.0.0` ⏩ `v93.1.1` --- ## [`v93.1.1`](https://github.com/elastic/eui/releases/v93.2.0) **This is a patch release primarily intended for use by Kibana.** - Added top-level `EuiTreeView.Item` export ([elastic#7526](elastic/eui#7526)) ## [`v93.1.0`](https://github.com/elastic/eui/releases/v93.1.0) - Added `index` glyph to `EuiIcon` ([elastic#7498](elastic/eui#7498)) - Updated `EuiHighlight` to accept an array of `search` strings, which allows highlighting multiple, separate words within its children. This new type and behavior *only* works if `highlightAll` is also set to true. ([elastic#7496](elastic/eui#7496)) - Updated `EuiContextMenu` with a new `panels.items.renderItem` property, which allows rendering completely custom items next to standard `EuiContextMenuItem` objects ([elastic#7510](elastic/eui#7510)) - `EuiSuperDatePicker` updates: - Updated `EuiSuperDatePicker` with a new `refreshIntervalUnits` prop. Passing this prop allows controlling and overriding the default unit rounding behavior. ([elastic#7501](elastic/eui#7501)) - Updated `EuiAutoRefresh` and `EuiRefreshInterval` with a new `intervalUnits` prop. Passing this prop allows controlling and overriding the default unit rounding behavior. ([elastic#7501](elastic/eui#7501)) - Updated `onRefreshChange` to pass back a new `intervalUnits` key that contains the current interval unit format (seconds, minutes, or hours). ([elastic#7501](elastic/eui#7501)) - Updated `EuiSuperDatePicker` with a new `canRoundRelativeUnits` prop, which defaults to true (current behavior). To preserve displaying the unit that users select for relative time, set this to false. ([elastic#7502](elastic/eui#7502)) - Updated `EuiSuperDatePicker` with a new `refreshMinInterval` prop, which accepts a minimum number in milliseconds ([elastic#7516](elastic/eui#7516)) - Updated `EuiAutoRefresh` and `EuiRefreshInterval` with a new `minInterval` prop, which accepts a minimum number in milliseconds ([elastic#7516](elastic/eui#7516)) **Bug fixes** - Fixed `EuiHighlight` to not parse `search` strings as regexes ([elastic#7496](elastic/eui#7496)) - Fixed `EuiSuperDatePicker` submit bug when used within `<form>` elements ([elastic#7504](elastic/eui#7504)) - Fixed an `EuiTreeView` bug where `aria-expanded` was being applied to items without expandable children ([elastic#7513](elastic/eui#7513)) **CSS-in-JS conversions** - Converted `EuiTreeView` to Emotion. Updates as part of the conversion: ([elastic#7513](elastic/eui#7513)) - Removed `.euiTreeView__wrapper` div node - Enforced consistent `icon` size based on `display` size
Summary
closes #6416
Opinionated changes as part of this conversion:
.euiTreeView__wrapper
(unnecessary extra div wrapper)icon
width/height is now enforced based ondisplay
/compressed sizingAs always, I recommend reviewing by commit.
https://github.com/elastic/kibana/blob/432a5d7734f127c321f75679be913a87bc3c71a6/x-pack/plugins/kubernetes_security/public/components/tree_view_container/dynamic_tree_view/index.tsx
The above is the highest-lift impact in Kibana that will need updating once this upgrade lands. They're apparently using the styles of EuiTreeView but not the actual component, which is partially why I split out a fairly stateless
EuiTreeViewItem
component for them to pull from and use directly.QA
display
andshowExpansionArrows
)General checklist
[ ] Updated the Figma library counterpart