-
Notifications
You must be signed in to change notification settings - Fork 843
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
Fix EuiNavDrawer collapse/expand button height issue #2463
Conversation
@cchaos This is what was happening when adding a background to the button on |
The only problem with |
Ok, I think I've found an alternative to using |
Where is the reference to this comment?:
|
I was refering to this https://github.com/elastic/eui/blob/v14.4.0/src/components/nav_drawer/nav_drawer.js#L332
|
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.
Ahh, right, ok I now follow through the comments on #2252.
LGTM! Only tested in Chrome. Can you just be sure to test in FF and IE before merging?
Just double checked on Firefox and IE11. |
Summary
I reverted some changes introduced in #2417 and ended up with the original situation (i.e. a collapse/expand button with a weird focus behaviour). However, I think I've found a way to fix this using the
:focus-within
selector.This is the behaviour now:
Fixes #2456
Note: I moved
footerContent
back to its original position (abovemodifiedChildren
) as I had missed a comment we have explaining why we need to have it beforemodifiedChildren
.Checklist
- [ ] Checked in dark mode- [ ] Checked in mobile- [ ] Props have proper autodocs- [ ] Added documentation examples- [ ] Added or updated jest tests- [ ] Checked for breaking changes and labeled appropriately- [ ] Checked for accessibility including keyboard-only and screenreader modes