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

Remove z-index and stacking contexts from list-items #3775

Merged
merged 6 commits into from
Jun 27, 2023
Merged

Conversation

bearfriend
Copy link
Contributor

@bearfriend bearfriend commented Jun 22, 2023

DE52324: Html emoji editor can get cut off by the rubric
US151482: Spike > Update d2l-list item rendering to avoid z-index

Removes all usage of z-index (and any internal stacking contexts) in list items to allow all descendants, and the list item itself, to stack organically within the document. This means no more special handling is needed to support open dropdowns, tooltips, or other overflowing descendants.

CSS order achieves essentially the same result as z-index, but it is scoped to the grid items and simply re-orders them instead of continually increasing their priority compared to everything else, so there is no arms race.

@bearfriend bearfriend changed the title Testing List item order Testing list item slot order Jun 22, 2023
@github-actions
Copy link
Contributor

Thanks for the PR! 🎉

We've deployed an automatic preview for this PR - you can see your changes here:

URL https://pr-3775-brightspace-ui-core.d2l.dev/

Note
The build needs to finish before your changes are deployed.
Changes to the PR will automatically update the instance.

@github-actions
Copy link
Contributor

Visual diff tests failed - pull request #3776 has been opened with the updated goldens.

@bearfriend bearfriend changed the title Testing list item slot order Remove z-index and stacking contexts from list-items Jun 22, 2023
@bearfriend bearfriend marked this pull request as ready for review June 22, 2023 22:26
@bearfriend bearfriend requested a review from a team as a code owner June 22, 2023 22:26
@@ -126,9 +114,10 @@ export const ListItemMixin = superclass => class extends composeMixins(
}

[slot="control-container"] {
pointer-events: none;
Copy link
Contributor Author

@bearfriend bearfriend Jun 22, 2023

Choose a reason for hiding this comment

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

position: relative; (necessary for the absolutely positioned borders below) seems to bump this item out of control of order, so I left it on top but disallowed pointer-events, which seems to work fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

No objection to changing this, just calling this out if we run into any issues with it. I don't think this z-index value was causing us any grief. It was just being used for rendering the separators in its ::before/::after pseudo-elements. The element itself is not actually a container for item content, and so would not be setting a stacking context for anything else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem was that without the parent stacking context, -1 puts it behind the list itself (or whatever it eventually hits) so the borders were no longer visible.

@github-actions
Copy link
Contributor

Visual diff tests failed - pull request #3777 has been opened with the updated goldens.

Copy link
Contributor

@dbatiste dbatiste 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!

@bearfriend bearfriend merged commit 6bc1fb4 into main Jun 27, 2023
5 checks passed
@bearfriend bearfriend deleted the list-item-order branch June 27, 2023 17:07
@ghost
Copy link

ghost commented Jun 27, 2023

🎉 This PR is included in version 2.131.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ghost ghost added the released label Jun 27, 2023
bearfriend added a commit that referenced this pull request Jun 27, 2023
* Switch list-item z-index to grid order

* Remove fullscreenWithin, dropdownOpen, and tooltipShowing

* Fix lint

* Remove fullscreen-within listener

* Try disabling pointer events

* Fix lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants