-
Notifications
You must be signed in to change notification settings - Fork 129
Migrate from MUI Swipeable Drawer to react-modal-sheet #3468
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
base: main
Are you sure you want to change the base?
Conversation
…ling and opacity control
… with wrong backdrop
|
@fbmcipher what is the reason we have a z-index on navbar? Is it possible to remove it? I can't add z-index to the drawer, since adding z-index creates a new stacking context, which breaks blending (ex. either progressive blur breaks, or sheet content blending with backdrop breaks). With all the blending in the new mocks, I think we are going to have to start using z-index sparingly, since it creates a new stacking context. Screen.Recording.2025-12-26.at.11.47.56.PM.mov |
|
@fbmcipher Also aside from the navbar issue, the new code allows the fade in only (no slide in) effect for the backdrop, and allows us to control opacity based on height of the drawer Screen.Recording.2025-12-27.at.12.07.57.AM.mov |
I don't think there is a way to avoid z-index, as we need to control the z order of absolutely positioned elements. We have a nice, centralized, global z-index schedule. The only other way I know to control z order is by source order (literally the order of elements in the DOM), which offers less control and can sometimes create impossible situations if there are other source ordering dependencies. |
@raineorshine I see, I did think of a way to get this to work with z-index, by moving the progressive blur outside the sheet. My issue now is that I can't get the @fbmcipher any ideas for this The blending on this done button seems to break really easily, since I've had to attempt to fix this button many times now. Is there another way we can do this? Ex. have an image for this button instead? |
|
Hi @yangchristina - thanks for sticking with this through all the challenges. Admittedly, it has proven more difficult than I expected to work with blending and compositing on the web, particularly on Safari. This issue stood out as strange to me, because the Using the Layers pane in the Safari developer tools, I noticed that the "Done" button isn't automatically being promoted to its own compositing layer. Each of the PanelCommand buttons, where blend modes do work, are on their own layer. Since the Done button blends as expected in Chrome, I used the Layers pane in Chrome DevTools to see if a compositing layer for the "Done" button is created there. Indeed, it is. So Safari is not promoting the Done button to its own compositing layer, whereas Chrome does so automatically. Not to be a shoddy workman blaming his tools, but this does look like yet another Safari rendering quirk that we need to anticipate and build for, and looks very similar to the issue we discussed in the Command Center buttons task. Proposed solutionSome brief testing showed that using <div
className={css({
gridArea: 'button',
background: 'fgOverlay20',
borderRadius: 46,
mixBlendMode: 'soft-light',
})}
style={{
// Safari fix: will-change forces GPU layer creation which fixes blend mode rendering bug.
willChange: 'transform',
}}
/>
<button
{...fastClick(onClose)}
className={css({
all: 'unset',
gridArea: 'button',
mixBlendMode: 'lighten',
opacity: 0.5,
fontWeight: 500,
cursor: 'pointer',
padding: '8px 16px',
})}
style={{
// Safari fix: will-change forces GPU layer creation which fixes blend mode rendering bug.
willChange: 'transform',
}}
>
Done
</button>In the thread linked above, we decided will-change should not be used frivolously, but in specific circumstances where we need to work around such rendering quirks, it is often the most performant solution to a problem. Here, we just use it to forcibly promote the "Done" button and its label to separate compositing layers and to match the behavior Chrome takes automatically. The impact on memory is measurable at about 150KB. As far as I can tell, as of this PR the Command Center's content is now unmounted when closed. So as soon as the Command Center is closed, those new layers will get removed and the memory will be freed up. So I think we can justify its use here – what do you think? cc @raineorshine |
|
Sure, that seems acceptable. |
|
@fbmcipher Thank you so much! For the
Does the |
|
@fbmcipher Right now, if you see the video inside the PR description, the active button glow fades in after the modal slides open. This is because it doesn't blend properly with the backdrop if it starts blending with the backdrop before the backdrop has even faded in. Is this acceptable? |
@yangchristina I see why you've implemented it this way, but I don't think this is quite what we want. Showing a button in a disabled-looking state and then transitioning it to active after a delay could be confusing to a user. We want state indication to feel consistent. It also breaks the sense of continuity we're trying to create throughout the app with this redesign.
Let's talk more about this, as I noticed some new code I bypassed
Is something like image 1 what you're referring to having seen in before implementing If so, this issue doesn't seem to be caused by This can be verified using
Toggling the favorite button off and on again sets the opacity to 0.75 as expected, so this seems to be an issue of the opacity not being set correctly on first render. Since Given this, I think the problem will be solved if we can figure out why |
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.
This generally looks great, and I'm really happy with the new drawer! I especially like the way the background glow is tied to the gesture, it feels really smooth.
Just a couple small bits from me.
| enterActive: { transition: `opacity {durations.medium} ease 0ms` }, | ||
| exitActive: { transition: `opacity {durations.medium} ease 0ms` }, | ||
| }, | ||
| commandCenterDrawer: { |
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 find myself missing the easing in the new drawer implementation. Without the easing, the animation feels a bit abrupt.
The easing added a subtle smoothness to the way the light and panel animate.
Is there a way for us to use the old easing curve with the new drawer?
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.
@fbmcipher is it okay to have the same easing for ease in and ease out? Here's with MUI's ease in, for both easing in and out.
Screen.Recording.2026-01-03.at.1.02.36.AM.mov
@fbmcipher I'm talking about the mix-blend-mode on the The mix-blend-mode (not will-change) on the |
|
@yangchristina Got you. Yeah, I don't see any difference with the |
…tate and props, and enhance fade transition with new appearance slots
@fbmcipher you are totally right! Thanks for noticing this! My bad for not seeing that. 797ea6d Screen.Recording.2026-01-03.at.12.27.40.AM.mov |
…ndCenter button styles
…n and enhance opacity transition with cubicBezier easing
… and update easing definition
|
not sure why tests are failing, they pass locally |
|
Could you look into the failing puppeteer test? |










Resolves #3283
IOS:
Screen.Recording.2026-01-02.at.12.50.05.PM.mov
Chrome:
Screen.Recording.2026-01-02.at.12.52.25.PM.mov