-
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
[EuiAccordion] Split up rendering into sub-components & other cleanups #7161
Conversation
- for easier dev readability and atomic styles + rename generic `iconButton` name to a more accurate `arrow` and remove className modifiers (use `[aria-expanded]` instead of `-isOpen` + inline some very minimal styles that don't require euiTheme or modifiers (trigger wrapper & optional action - action needs a label to trigger emotion snapshot serializer logic)
- order imports by specificity - order destrucuted props by approximate usage - remove unnecessary newlines
- allows us to use more succinct useEffect/useRefs + remove extra unnecessary div wrapper (not really sure what it was even doing) + remove extra loading styles in favor of a quick inline one-liner
…rectly - this feels more react-y and has the benefit of not requiring an extra useEffect on mount
…instead - see https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/inert - this property completely does what we want and full browser support was added in early 2023 - also allows us to remove `visibility` CSS which we were using to hide content from screen readers
- remove CSS overrides in favor of two separate states - add missing motion media query around height/opacity transition - remove complete focus removal in favor of `:focus-visible` selector and existing mixin
Preview documentation changes for this PR: https://eui.elastic.co/pr_7161/ |
b673bde
to
64ae98f
Compare
64ae98f
to
e195f62
Compare
Preview documentation changes for this PR: https://eui.elastic.co/pr_7161/ |
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 refactor was well needed and you broke the subcomponents up very well. This component is complex and keeping all of the moving pieces is hard when everything is intertwined.
- The new
inert
attribute REALLY did wonders here! It's very cool to see us remove a huge chunk of manual logic in favor of built in browser support. - I just have one question in regards to Emotion sprinkled in.
- I checked production to confirm accordions work as they did before. I also checked Storybook for the edge case to ensure that hidden children are not tabbed.
90deg | ||
) !important; /* stylelint-disable-line declaration-no-important */ | ||
`, | ||
right: css` |
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.
Do you think it would be helpful to have an empty left
key here for consistency? Left doesn't have any styles, but having the empty key would be a reminder that it's an option?
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.
For sure, I love it! I'll add it here in a bit
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.
There actually is CSS to put in the left styles! It avoids making right
override left
🎉
This feels like a huge improvement, thank you for the awesome suggestion Bree!
buttonElement, | ||
buttonProps, | ||
buttonClassName, | ||
buttonContentClassName, | ||
buttonContent, | ||
arrowDisplay, | ||
arrowProps, |
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.
We should document the standard of "how to order imports" as a rough guideline in our How We Work wiki
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 is a super interesting (and potentially controversial 😅) topic of discussion actually. Personally, I work with the mental model of "group related props together", and it makes sense to me in my brain, but I've worked with devs in the past where that just doesn't make any sense to them and they strongly strictly prefer ordering EVERYTHING (imports, props, all object keys, etc.) alphabetically, and lint for it.
I'm going to be honest, I'm... not a huge fan of alphabetical sorting (there's just too many one-offs where related props end up far apart from each other, and it kinda relies on verbose or strict naming conventions). But yeah this is basically a topic where opinions can get a little heated and it can lead to a frustrating amount of bikeshedding, so all-in-all I've kinda just gone with the laissez-faire approach of "I'll clean it up if it's in my PR and I'm already here", as opposed to pushing for stricter linting around it.
It's definitely an interesting topic to discuss as a team (if it doesn't turn out into a spaces vs tabs holy war lol) so I'll put it on our next sync to quickly chat about if we're down for it!
const [contentHeight, setContentHeight] = useState(0); | ||
const onResize = useCallback( | ||
({ height }: { height: number }) => setContentHeight(Math.round(height)), | ||
[] | ||
); | ||
const heightInlineStyle = useMemo( | ||
() => ({ blockSize: isOpen ? contentHeight : 0 }), | ||
[isOpen, contentHeight] | ||
); |
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.
Very nice refactor!
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.
Huzzah, great to hear it! In general I'm a huge fan of how function components and hooks makes us write more modern React best practices :)
/** | ||
* Ensure accordion children are correctly removed from tabindex order | ||
* when accordions are closed, and correctly restored on open | ||
*/ | ||
const tabbableChildren = useRef<FocusableElement[] | null>(null); | ||
|
||
useEffect(() => { | ||
// When accordions are closed, tabbable children should not be present in the tab order | ||
if (!isOpen) { | ||
// Re-check for children on every close - content can change dynamically | ||
tabbableChildren.current = tabbable(wrapperRef.current!); | ||
|
||
tabbableChildren.current.forEach((element) => { | ||
// If the element has an existing `tabIndex` set, make sure we can restore it | ||
const originalTabIndex = element.getAttribute('tabIndex'); | ||
if (originalTabIndex) { | ||
element.setAttribute('data-original-tabindex', originalTabIndex); | ||
} | ||
|
||
element.setAttribute('tabIndex', '-1'); | ||
}); | ||
} else { | ||
// On open, restore tabbable children | ||
// If no tabbable children were set, we don't need to re-enable anything | ||
if (!tabbableChildren.current) return; | ||
|
||
tabbableChildren.current.forEach((element) => { | ||
const originalTabIndex = element.getAttribute('data-original-tabindex'); | ||
if (originalTabIndex) { | ||
// If the element originally had an existing `tabIndex` set, restore it | ||
element.setAttribute('tabIndex', originalTabIndex); | ||
element.removeAttribute('data-original-tabindex'); | ||
} else { | ||
// If not, remove the tabIndex property | ||
element.removeAttribute('tabIndex'); | ||
} | ||
}); | ||
// Cleanup - unset the list of children | ||
tabbableChildren.current = null; | ||
} | ||
}, [isOpen]); | ||
|
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.
Imagine one new attribute saving us from this! TIL about inert
.
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.
HAHA yeah I definitely had a "lol omg f all that work then" moment when I found the inert
property (from reading through tabbable
's docs!) But frankly, I always LOVE seeing code get deleted and this is way more performant, so I'm very happy with the change!
- add `left` and `right` CSS keys - add open/closed CSS keys while we're here - update snapshots
Preview documentation changes for this PR: https://eui.elastic.co/pr_7161/ |
💚 Build Succeeded
History
|
EUI `88.2.0` ➡️ `88.3.0` ## [`88.3.0`](https://github.com/elastic/eui/tree/v88.3.0) - `EuiGlobalToastList` now shows a "Clear all" button by default once above a certain number of toasts (defaults to 3). This threshold is configurable with the `showClearAllButtonAt` prop ([#7111](elastic/eui#7111)) - Added an optional `onClearAllToasts` callback to `EuiGlobalToastList` ([#7111](elastic/eui#7111)) - Added the `value`, `onChange`, and `onCancel` props that allow `EuiInlineEdit` to be used as a controlled component ([#7157](elastic/eui#7157)) - Added `grabOmnidirectional`, `transitionLeftIn`, `transitionLeftOut`, `transitionTopIn`, and `transitionTopOut` icon glyphs. ([#7168](elastic/eui#7168)) **Bug fixes** - Fixed `EuiInlineEdit` components to correctly spread `...rest` attributes to the parent wrapper ([#7157](elastic/eui#7157)) - Fixed `EuiListGroupItem` to correctly render the `extraAction` button when `showToolTip` is also passed ([#7159](elastic/eui#7159)) **Dependency updates** - Updated `@hello-pangea/dnd` to v16.3.0 ([#7125](elastic/eui#7125)) - Updated `@types/lodash` to v4.14.198 ([#7126](elastic/eui#7126)) **Accessibility** - `EuiAccordion` now correctly respects reduced motion settings ([#7161](elastic/eui#7161)) - `EuiAccordion` now shows a focus outline to keyboard users around its revealed children on open ([#7161](elastic/eui#7161)) **CSS-in-JS conversions** - Converted `EuiSplitPanel` to Emotion ([#7172](elastic/eui#7172))⚠️ As a quick heads up, serverless tests appear to have been extremely flake/failure-prone the last couple weeks, particularly Cypress tests. We've evaluated the listed failures and fixed ones that were related to changes in this PR, and we're relatively confident the remaining failures are not related to changes from EUI. Please let us know if you think this is not the case. --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Cee Chen <constance.chen@elastic.co> Co-authored-by: Jon <jon@elastic.co>
⚠️ NOTE: This PR is a copy of #166292 (which was reverted due to failing Storybook builds). This is the same exact PR but with Storybook building fixed. --- EUI `88.2.0` ➡️ `88.3.0` ## [`88.3.0`](https://github.com/elastic/eui/tree/v88.3.0) - `EuiGlobalToastList` now shows a "Clear all" button by default once above a certain number of toasts (defaults to 3). This threshold is configurable with the `showClearAllButtonAt` prop ([#7111](elastic/eui#7111)) - Added an optional `onClearAllToasts` callback to `EuiGlobalToastList` ([#7111](elastic/eui#7111)) - Added the `value`, `onChange`, and `onCancel` props that allow `EuiInlineEdit` to be used as a controlled component ([#7157](elastic/eui#7157)) - Added `grabOmnidirectional`, `transitionLeftIn`, `transitionLeftOut`, `transitionTopIn`, and `transitionTopOut` icon glyphs. ([#7168](elastic/eui#7168)) **Bug fixes** - Fixed `EuiInlineEdit` components to correctly spread `...rest` attributes to the parent wrapper ([#7157](elastic/eui#7157)) - Fixed `EuiListGroupItem` to correctly render the `extraAction` button when `showToolTip` is also passed ([#7159](elastic/eui#7159)) **Dependency updates** - Updated `@hello-pangea/dnd` to v16.3.0 ([#7125](elastic/eui#7125)) - Updated `@types/lodash` to v4.14.198 ([#7126](elastic/eui#7126)) **Accessibility** - `EuiAccordion` now correctly respects reduced motion settings ([#7161](elastic/eui#7161)) - `EuiAccordion` now shows a focus outline to keyboard users around its revealed children on open ([#7161](elastic/eui#7161)) **CSS-in-JS conversions** - Converted `EuiSplitPanel` to Emotion ([#7172](elastic/eui#7172)) --------- Co-authored-by: Bree Hall <briannajdhall@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Jon <jon@elastic.co>
Summary
As always, I recommend following along by commit and reading the full commit messages!
Things this PR does
render()
method ofEuiAccordionClass
, and splits up styles alongside their sub-components.euiAccordion__iconButton
to a more descriptive.euiAccordion__arrow
(and removes the-isOpen
modifier classes in favor of[aria-expanded="true"]
):focus-visible
outline around EuiAccordion children on open to make it more visibly clearer that focus has been moved<div>
wrapper around accordion childrentabbable()
logic I added in [EuiAccordion] Improve keyboard accessibility of tabbable children #7064. I realized while working on this that the inert property (which received full cross-browser support earlier this year in spring 2023) does exactly what we want without all that extra code 🤦Things this PR should not do
accordion.test.tsx
andaccordion.spec.tsx
files to regression test for usQA
gh pr checkout 7161 && yarn storybook
General checklist
and cypress testsand screenreader modes[ ] Checked for breaking changes and labeled appropriately- className change is a minor breaking change, I'll handle this myself in the next Kibana upgrade- [ ] Checked in both light and dark modes- [ ] Checked in mobile- [ ] Props have proper autodocs (using@default
if default values are missing) and playground toggles- [ ] Added documentation- [ ] Checked Code Sandbox works for any docs examples- [ ] Updated the Figma library counterpart