Skip to content
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] Allow interactive content within the buttonContent #5258

Merged
merged 15 commits into from
Oct 19, 2021

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Oct 11, 2021

This is needed to allow our EuiCollapsibleNavGroup titles to navigate directly to Kibana's overview pages while keeping the collapse toggle on all the parts but the interactive content.

While looking through the component code, I saw a couple places where I could tell it was really old-gen code and could use an update.

1. Arrow icon

Before, we were displaying the arrow as just a simple EuiIcon, that we then wrapped in a <button> when displayed on the right with an extraAction. Which we then applied custom interaction styles on top of. I replaced this logic by just always rendering an EuiButtonIcon but moving it out of the buttonContent and removing from the tab-order if the buttonContent is a <button>. (more on that later).

It does mean that the focus outline is now only around the text of the button content, but it fixes the weird shape that sometimes happens and the arrow itself has consistent hover states to our normal icon buttons.

Screen Shot 2021-10-11 at 14 25 52 PM

I also added arrowProps?: Partial<EuiButtonIconProps> that gets passed to this button.

2. Easily create a fieldset/legend combo with new element prop

Right now this prop is purely restricted to div | fieldset (div is default) , but can be expanded as we need. When the accordion is provided fieldset, it will automatically convert the buttonContent's wrapping element to a legend. And updated the "Styled form form" section to use the element = fieldset setting.

Screen Shot 2021-10-06 at 17 52 58 PM

This helps tremendously with VO when focusing into an input contained within an accordion.

Screen Shot 2021-10-11 at 14 37 41 PM

3. Added buttonElement to force to a div to allow for interactive content

Right now the only options are 'div' | 'legend' | 'button'. But when it is not a button, it will force the arrow display as the focusable toggle for the accordion.

Screen Shot 2021-10-11 at 14 44 47 PM

I also updated the Collapsible Nav -> 'Full pattern with header and saved pins' example to better match what is being implemented in Kibana, including the "Add data" button at the bottom and updated "Manage deployment" button with the ghost Cloud icon. It's a good place to test the real-world "link inside the accordion header" functionality.

Screen Shot 2021-10-11 at 14 17 59 PM

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs and playground toggles
  • Added documentation
  • Checked Code Sandbox works for any docs examples
  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5258/

@cchaos
Copy link
Contributor Author

cchaos commented Oct 11, 2021

This PR is reviewable now with the caveat that I still have work to do on the rotation animation of the arrow button.

@cchaos cchaos marked this pull request as ready for review October 11, 2021 21:08
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5258/

@cchaos cchaos added this to the Elastic Stack 8.0 milestone Oct 12, 2021
cchaos added 2 commits October 13, 2021 18:03
…e_nav_accordion

# Conflicts:
#	src-docs/src/views/accordion/accordion.js
#	src-docs/src/views/accordion/accordion_form.js
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5258/

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functional and a11y tested. LGTM

I still have work to do on the rotation animation of the arrow button.

Still working on this?

src/components/accordion/accordion.tsx Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@cchaos
Copy link
Contributor Author

cchaos commented Oct 18, 2021

Thanks for my own note reminder 😂 . Fixed.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5258/

@cchaos cchaos merged commit 4c21d16 into elastic:master Oct 19, 2021
@cchaos cchaos deleted the update/collapsible_nav_accordion branch October 19, 2021 15:36
ym pushed a commit to ym/eui that referenced this pull request Oct 29, 2021
…lastic#5258)

* Make accordion arrows always EuiButtonIcons
* Added `element` and `buttonElement` to customize HTML elements
* Added `arrowProps` so collapsible group could change its color for dark
* Hard coded example of stopPropagation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants