-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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(ListBox): change ListBox z-index higher than modal #8107
Conversation
Deploy preview for carbon-elements ready! Built with commit b8864ec |
Deploy preview for carbon-components-react ready! Built with commit b8864ec https://deploy-preview-8107--carbon-components-react.netlify.app |
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.
since this issue has swung back and forth it might be worth specifically targeting components within a modal for a z-index bump rather than re-reverting a global variable (i.e. modal z-index > dropdown z-index but dropdown z-index within a modal > modal?). it looks like this same concern was raised in #2660 as well
I'd be curious why the cleanup reverted the list-box to below the modal in the first place - It seems logical to me that floating menus and lists should occupy the top of the z-index range, with modals and other "windows" sitting somewhere below. As far as I understand menus and dropdowns should close when a modal opens (even just by virtue of it being an outside click), so that shouldn't cause a layering conflict. This particular issue occurs because of what's required to break out of a modal - the list-box has to be rendered into the body. I'm not sure how you would specifically target the list-box when it's not a sibling of the dropdown, or a child of the modal? As mentioned in #2660, the whole z-index order probably should be reworked a bit anyway - setting it to 9100 was a minimally breaking hack. |
Perhaps commenting why the z-indices are what they are in the code would help prevent this from regressing unnecessarily? |
dbc417d
to
d4b9bba
Compare
@cal-smith agreed, z-index's should probably be revisited as a whole, but added a comment for now. I didn't change @emyarod due to the way the Angular team is rendering Dropdowns (child of |
d4b9bba
to
0a1c168
Compare
0a1c168
to
b8864ec
Compare
Refs #2660
Refs #6365
This stems from a conversation on Slack from @carbon-design-system/angular-maintainers and you can see the issue occurring here. This was previously fixed by the Angular team in #2660 but reverted in #6365
Updates
ListBox
elements z-index values to aboveModal
, so that a Dropdown rendered outside of the Modal content (to prevent Dropdown causing scrolling inside Modal) will appear above the Modal itself. This should not cause any issues when triggering a Modal when aListBox
element is open, since the menu's all close when the focus is removed. Let me know if I am missing a particular use-case.Changelog
Changed
dropdown
value from6000
to9100
Testing / Reviewing
z('dropdown')
is only used inDropdown
andListbox
components