-
Notifications
You must be signed in to change notification settings - Fork 535
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
Add isolation:isolate
to ButtonGroup
container
#2910
Conversation
🦋 Changeset detectedLatest commit: e9f6ccc The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
Loved your explanation for this @iansan5653, thanks for taking the time to write that up 🙏 Do you know if there is a good lint rule or test that we could include to try and catch this stuff across components? Would love to make sure |
🤔 I think it would be pretty tricky to figure out how to lint for this. You might be able to test for it by walking up the DOM tree and checking for a new stacking context but even that would be tricky to get right. Alternatively we could lint for all instances of There is an open tech debt issue in Memex on the same topic where I outline a set of rules that we can universally follow. We can document this in a flowchart: flowchart TD
A[Does this need to render<br> over the entire application?]
A -->|Yes| B
A -->|No| C
B(Use <code>Portal</code> to render into<br> the default <code>primerPortalRoot</code>)
C[Can you rearrange the DOM to<br> control stacking?]
C -->|Yes| D
C -->|No| E
D(Render the element that should<br> be on top last in the DOM)
E(Use <code>z-index</code> and also apply <code>isolation: isolate</code><br> to the component's container)
Alternatively, maybe we can create a new Primer abstraction to help with this? Just thinking out loud here, but maybe some sort of context provider that also acts as a stacking context container, like so: const AvatarStackingContext = createStackingContext()
const AvatarStack = (users) => (
<AvatarStackingContext.Container>
{users.map((user, i) => (
<AvatarStackingContext.Layer level={users.length - i} key={user.login}>
<Avatar user={user} />
</AvatarStackingContext.Layer>
)}
</AvatarStackingContext.Container>
) It's verbose, but maybe it's worth it for the explicit management of stacking here? Maybe we can make it more intuitive with named layers, or validate the layers somehow (maybe only allow each layer number to be used once). Then we could ban |
Using
z-index
in reusable components is generally a bad idea, becausez-index
is one of the most subtle side-effecting CSS rules in existence. It will inevitably break things down the road. The only way to usez-index
internally without affecting surrounding UI is to create a new stacking context to isolate the effectsIn
ButtonGroup
, thez-index
on the focused element is necessary to be able to show the focused button's outline above the other buttons. So we can't remove it altogether. But we can definitely isolate it, and there's no reason not to - the only goal here is to make the element show above its fellow buttons - not above the entire app. The easiest way to make a new stacking context with no other effects isisolation: isolate
.Fixes https://github.com/github/memex/issues/8722