-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[core] fix(Drawer): allow clicking on overlaid contents when hasBackdrop=false #6650
Conversation
#{example("Drawer")} { | ||
.docs-example > * { | ||
margin: 0; |
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.
extract className from propsBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
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 seems to work well and it is a good improvement over the default. It is a behavior change compared to the current <Drawer hasBackdrop={false}>
(which can sometimes be considered breaking), but I think it will be a welcome one. Users are already able to scroll the contents behind a drawer when hasBackdrop={false}
:
this just makes it so that they can also click on the content underneath, as you demonstrated above 👍
@@ -130,10 +131,10 @@ export class Drawer extends AbstractPureComponent<DrawerProps> { | |||
[isPositionHorizontal(realPosition) ? "height" : "width"]: size, | |||
}; | |||
return ( | |||
<Overlay {...this.props} className={Classes.OVERLAY_CONTAINER}> | |||
<Overlay {...overlayProps} className={classNames({ [Classes.OVERLAY_CONTAINER]: hasBackdrop })}> |
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 realize now that this "overlay container" class (added in #2957) has a somewhat misleading name since it's only used by the Drawer component and it's non-descriptive about its purpose... it might make more sense to call this "OVERLAY_FIXED_BACKDROP" or something like that. We can't change it now though; it's part of the public API... just musing about the name here, perhaps I'll add some code comments about this in a separate PR.
Fixes #3515
Checklist
Changes proposed in this pull request:
Remove the modal trait of Drawer when
hasBackdrop
is set tofalse
by making overlay container styles conditional to
hasBackdrop
.Reviewers should focus on:
canOutsideClickClose
becomes ignore whenhasBackdrop
is falseScreenshot
New: clicking outside the drawer discards it and changes the position selection
Old: clicking outside the drawer discards it. An additional click is required to change the position selection