-
Notifications
You must be signed in to change notification settings - Fork 829
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
[EuiResizableButton] Add new prop to allow consumers to account for overlap with scrollbars #8021
Conversation
* This can occasionally occlude interactive elements like scrollbars. To prevent | ||
* this overlap, use this prop to remove the overlap for the specified side. | ||
*/ | ||
accountForScrollbars?: 'before' | 'after' | 'both'; |
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 open to feedback on this prop name/API - I'm not 100% sold on it or anything but it was the best I could come up with 😅
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.
Hmm, yeah it's kind of very specific but at the same time it might be not clear enough otherwise ...
Maybe accountForOverlap
? 🤔
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'm going to opt to merge this in as-is if that's okay! Overlap
also feels a little vague to me, at least Scrollbars
hints as to the purpose of the prop? 😅
💚 Build Succeeded
|
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.
LGTM 👍
I added a suggestion for the naming, which might be slightly less specific, but it's non-blocking.
* This can occasionally occlude interactive elements like scrollbars. To prevent | ||
* this overlap, use this prop to remove the overlap for the specified side. | ||
*/ | ||
accountForScrollbars?: 'before' | 'after' | 'both'; |
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.
Hmm, yeah it's kind of very specific but at the same time it might be not clear enough otherwise ...
Maybe accountForOverlap
? 🤔
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.
LGTM! I'm good with the prop name you have as well.
Summary
Follow up to #8010 (comment) and #7988
This PR adds the
accountForScrollbars
prop to EuiResizableButton, which allows consumers to conditionally remove the negative margins behavior on the button if it causes the button to block scrollbars.QA
General checklist
- [ ] Checked in both light and dark modes- [ ] Checked in mobileand screenreader modes@default
if default values are missing) and playground toggles- [ ] Checked Code Sandbox works for any docs examples- [ ] Added or updated jest and cypress tests- [ ] Updated visual regression tests- [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)