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] Fix accordion right arrow display when combined with extraAction #3971

Merged
merged 19 commits into from
Sep 3, 2020

Conversation

TAYTS
Copy link
Contributor

@TAYTS TAYTS commented Aug 28, 2020

Summary

Fixed #3881: EuiAccordionarrow display right with extra action styling.

Screenshot 2020-08-28 at 10 07 07 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
  • Added documentation
  • Checked Code Sandbox works for the any docs examples
  • Added or updated jest 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

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@TAYTS TAYTS force-pushed the feature/fix-accordion-arrow-display branch from 039e293 to dbad7fa Compare August 28, 2020 15:18
@thompsongl
Copy link
Contributor

Thanks, @TAYTS
Is there still work to be done on this PR?

When it's ready please mark as 'Ready for review' and check or strikethrough the checklist items to reflect what you've done (this will need a changelog entry).

@cchaos
Copy link
Contributor

cchaos commented Aug 31, 2020

I think they're waiting on an answer from me to a question they posed in the issue: #3881 (comment)

@cchaos
Copy link
Contributor

cchaos commented Aug 31, 2020

Jenkins, test this

@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor

cchaos commented Aug 31, 2020

Answering questions from #3881 (comment)

Detail 1: Is it ok for the buttonClassName prop to only apply to the text button and exclude the icon?

The problem with that is that it will break the visual for anyone using buttonClassName to style the accordion to their needs. I would say it is ok to move the arrow out of the buttonClassName only if the position for the arrow is on the right or none.

Detail 2: Is it ok for the icon highlight when focus to be separated from the text button?

Yes, testing in your current implementation, when the icon is on the right, it's ok to not show the focus ring.


I did notice, however, that in this PR, the none option is showing a placement and focus of an arrow that doesn't exist.

Screen Shot 2020-08-31 at 15 51 52 PM

@TAYTS TAYTS marked this pull request as ready for review September 1, 2020 06:44
@cchaos
Copy link
Contributor

cchaos commented Sep 1, 2020

Jenkins, test this

@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor

cchaos commented Sep 2, 2020

Jenkins, test this

@kibanamachine
Copy link

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

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Nice! All the logic looks great. Thanks for handling the accessibility portions as well. My comments are mostly patterns we like to follow in EUI specifically. They should be easy to follow, but if I didn't explain well, just holler.

You'll also need to update the jest snapshots.

CHANGELOG.md Outdated Show resolved Hide resolved
src-docs/src/views/accordion/accordion_extra.js Outdated Show resolved Hide resolved
src/components/accordion/accordion.tsx Outdated Show resolved Hide resolved
src/components/accordion/_accordion.scss Outdated Show resolved Hide resolved
@TAYTS TAYTS requested a review from cchaos September 2, 2020 17:00
@TAYTS TAYTS force-pushed the feature/fix-accordion-arrow-display branch from 0c9c475 to 272dfc6 Compare September 3, 2020 07:47
@cchaos
Copy link
Contributor

cchaos commented Sep 3, 2020

Jenkins, test this

@kibanamachine
Copy link

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

- EuiAccordion is used in EuiCollapsibleNavGroup
@cchaos
Copy link
Contributor

cchaos commented Sep 3, 2020

Jenkins, test this

@kibanamachine
Copy link

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

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

🎉 Looks great! Thanks for making all those changes. I also checked the screen-reader readout and it works properly when focused.

@cchaos cchaos changed the title Feature/fix accordion arrow display [EuiAccordion] Fix accordion right arrow display when combined with extraAction Sep 3, 2020
@cchaos cchaos merged commit f35352c into elastic:master Sep 3, 2020
@legrego
Copy link
Member

legrego commented Sep 3, 2020

Thanks so much for this, @TAYTS!

@TAYTS TAYTS deleted the feature/fix-accordion-arrow-display branch September 4, 2020 02:36
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.

EuiAccordion does not respect arrowDisplay="right" when extraAction is configured
5 participants