feat(react-dialog): add appearance to backdrop slot #35692
Conversation
📊 Bundle size report
Unchanged fixtures
|
|
Pull request demo site: URL |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -0,0 +1,7 @@ | |||
| { | |||
There was a problem hiding this comment.
🕵🏾♀️ visual changes to review in the Visual Change Report
vr-tests-react-components/Dialog 16 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-react-components/Dialog.dialog inside drawer default - Dark Mode.chromium.png | 0 | Added |
| vr-tests-react-components/Dialog.dialog inside drawer default - High Contrast.chromium.png | 0 | Added |
| vr-tests-react-components/Dialog.dialog inside drawer default - RTL.chromium.png | 0 | Added |
| vr-tests-react-components/Dialog.dialog inside drawer default.chromium.png | 0 | Added |
| vr-tests-react-components/Dialog.dialog inside drawer dimmed - Dark Mode.chromium.png | 0 | Added |
| vr-tests-react-components/Dialog.dialog inside drawer dimmed - High Contrast.chromium.png | 0 | Added |
| vr-tests-react-components/Dialog.dialog inside drawer dimmed - RTL.chromium.png | 0 | Added |
| vr-tests-react-components/Dialog.dialog inside drawer dimmed.chromium.png | 0 | Added |
| vr-tests-react-components/Dialog.dialog inside drawer transparent - Dark Mode.chromium.png | 0 | Added |
| vr-tests-react-components/Dialog.dialog inside drawer transparent - High Contrast.chromium.png | 0 | Added |
| vr-tests-react-components/Dialog.dialog inside drawer transparent - RTL.chromium.png | 0 | Added |
| vr-tests-react-components/Dialog.dialog inside drawer transparent.chromium.png | 0 | Added |
| vr-tests-react-components/Dialog.nested dialog dimmed - Dark Mode.chromium.png | 0 | Added |
| vr-tests-react-components/Dialog.nested dialog dimmed - High Contrast.chromium.png | 0 | Added |
| vr-tests-react-components/Dialog.nested dialog dimmed - RTL.chromium.png | 0 | Added |
| vr-tests-react-components/Dialog.nested dialog dimmed.chromium.png | 0 | Added |
vr-tests-react-components/Positioning 2 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-react-components/Positioning.Positioning end.updated 2 times.chromium.png | 511 | Changed |
| vr-tests-react-components/Positioning.Positioning end.chromium.png | 611 | Changed |
vr-tests-react-components/TagPicker 3 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-react-components/TagPicker.disabled - RTL.disabled input hover.chromium.png | 635 | Changed |
| vr-tests-react-components/TagPicker.disabled - Dark Mode.disabled input hover.chromium.png | 658 | Changed |
| vr-tests-react-components/TagPicker.disabled.chromium.png | 677 | Changed |
There were 3 duplicate changes discarded. Check the build logs for more information.
|
TBH, I'm not sure I understand the purpose of the @mainframev, have you considered adding a prop to prevent the backdrop from being transparent, even when the dialog is nested? This might be clearer than using |
mainframev
force-pushed
the
fix/drawer-dialog-dimming-issue
branch
from
79c1a81 to
946c759
Compare
mainframev
changed the title
mainframev
force-pushed
the
fix/drawer-dialog-dimming-issue
branch
from
946c759 to
be42731
Compare
mainframev
force-pushed
the
fix/drawer-dialog-dimming-issue
branch
from
be42731 to
8dd2d1a
Compare
Yeah, I agree that it was not very well thought through very well. I thought in the direction of adding a This would be a convenient prop that makes users lives easier, especially since we have already received several questions about this. Alternatively, we could approach this differently by exposing a data attribute on the backdrop slot, for example: backdrop={{ 'data-backdrop-appearance': 'dimmed' }}. This could help avoid potential confusion caused by having both backdrop and backdropAppearance props at the same time. |
mainframev
force-pushed
the
fix/drawer-dialog-dimming-issue
branch
3 times, most recently
from
2985d80 to
2d44af2
Compare
mainframev
changed the title
mainframev
force-pushed
the
fix/drawer-dialog-dimming-issue
branch
from
f5ce3f2 to
45c0108
Compare
mainframev
changed the title
mainframev
force-pushed
the
fix/drawer-dialog-dimming-issue
branch
from
da952d2 to
4f2ca66
Compare
dmytrokirpa
left a comment
dmytrokirpa
left a comment
There was a problem hiding this comment.
LGMT, just two things to address and should be good to go:
- Clarify why do we need to remove the
appearanceprop from thebackdropslot - Could we add some tests (VR, e2e, etc) to test the old/new behavior?
| backdrop.className, | ||
| ); | ||
|
|
||
| if (backdrop?.appearance) { |
There was a problem hiding this comment.
I was not fully sure about that, but this is needed in order to prevent appearance to be added to DOM element as attribute, I think something similar we have with Tree component. The utility we use to whitelist the attributes is gonna be removed according to your proposal, so I left this approach.
Could we add some tests (VR, e2e, etc) to test the old/new behavior?
👍🏻
There was a problem hiding this comment.
I see, so we basically need to remove a custom prop from a 'div' slot before rendering.
The confusing part is that we're doing this in a "style" hook 😕
2 participants
This pull request introduces a new
appearanceproperty to theDialogSurfacebackdropslot in@fluentui/react-dialog, enabling explicit control over the dialog's backdrop appearance. By default, the dialog automatically chooses between a dimmed or transparent backdrop based on its nesting context, but this change allows developers to override that behavior. The update includes type definitions, logic for resolving the backdrop appearance, style adjustments, documentation, and a new story demonstrating the feature.DialogBackdropSlotPropstype to support anappearanceprop ('dimmed'or'transparent') for the backdrop slot inDialogSurface.Previous Behavior
The dimmed background applied based on the Dialog context. Inside OverlayDrawer dialogs become nested and inner Dialog does not have dimmed background. The only way to apply the dimmed background for inner Dialog is via
backdropslot inDialogSurfaceNew Behavior
Can be controlled via
backdropAppearancewithout the need to use hack with passing custom styled backdrop toDialogSurfaceRelated Issue(s)
Related older discussion