[Overlay] New Overlay CSS plays nice with centered, scrollable dialogs #1373

llorca · 2017-07-21T22:14:32Z

Fixes #552, Fixes #1240, Fixes #1313

Checklist

Include tests
Update documentation

Reviewers should focus on:

New overlay CSS
Updated dialog CSS

What happened

Dialog component now renders a .pt-dialog-container wrapper as its root element which centers the .pt-dialog in a viewport-covering flex container. This provides super reliable centering behavior (#552) and much improved scrolling support (though some issues are still present like #1008).

revert changes to Overlay, fix test, use rAF for popover focus logic

mostly reducing duplication!

blueprint-bot · 2017-07-21T23:04:26Z

Fix Sass linting

Preview: documentation
Coverage: core | datetime

cmslewis · 2017-07-24T21:24:30Z

Take a look at the Overlay example: the background color is flickering and ending up darker after a moment:

cmslewis

Code looks good to me.

llorca · 2017-07-24T21:30:46Z

@cmslewis Hmm, what browser? I can't repro on Chrome nor Firefox

blueprint-bot · 2017-07-24T21:39:50Z

Disable text selection on backdrop

Preview: documentation
Coverage: core | datetime

llorca · 2017-07-24T22:01:31Z

Just added user-select: none to the backdrop. It fixes this bug in Firefox where mousing down on the backdrop selects the text below:

blueprint-bot · 2017-07-24T22:20:19Z

Even out toast margin / toast container padding

Preview: documentation
Coverage: core | datetime

giladgray

looks good, we did a great job here! ✋
a few little cleanups and should be good to go

giladgray · 2017-07-25T19:37:24Z

packages/core/src/components/context-menu/contextMenu.tsx

    }

    private cancelContextMenu = (e: React.SyntheticEvent<HTMLDivElement>) => e.preventDefault();

    private handleBackdropContextMenu = (e: React.MouseEvent<HTMLDivElement>) => {
-        // HACKHACK: React function to remove from the event pool (not sure why it's not in typings #66)
-        (e as any).persist();
+        // HACKHACK: React function to remove from the event pool


actually this is not a HACKHACK, it's expected usage if you want to use an event in a callback. can you remove the HACKHACK keyword but leave the rest of comment?

giladgray · 2017-07-25T19:37:53Z

packages/core/src/components/dialog/_dialog.scss

-  z-index: $pt-z-index-overlay;
-  // add bottom margin for overflow scrolling scenarios
-  margin-bottom: $pt-grid-size * 2;
+  margin: ($pt-grid-size * 3) 0;


$dialog-margin please

giladgray · 2017-07-25T19:38:05Z

packages/core/src/components/dialog/dialog.tsx

-                <div className={classNames(Classes.DIALOG, this.props.className)} style={this.props.style}>
-                    {this.maybeRenderHeader()}
-                    {this.props.children}
+                <div className="pt-dialog-container">


Classes.DIALOG_CONTAINER

giladgray · 2017-07-25T19:39:03Z

packages/core/src/components/overlay/overlay.md

@@ -56,4 +56,4 @@ and we'll take care of the rest.
 <Overlay className="pt-overlay-scroll-container" ... />
 ```

-The `Dialog` component does this automatically internally, except when it is used `inline`.
+The `Dialog` component applies this CSS class automatically internally.


remove word "internally"

giladgray · 2017-07-25T19:39:20Z

packages/core/src/components/overlay/overlay.tsx

+                    ref={this.refHandlers.container}
+                >
+                    {transitionGroup}
+                </span>


should be safe to undo this formatting, nothing actually changed here.

Much easier to parse next to the corresponding else block right after, don't you think?

can't see that other block in the default diff here but you're right

giladgray · 2017-07-25T19:39:47Z

packages/core/src/components/popover/_popover.scss

@@ -120,6 +120,10 @@ $popover-width: $pt-grid-size * 35 !default;
  .pt-transition-container {
    @include popover-inline-position();
  }
+
+  .pt-overlay-inline {


can you add a comment why this is necessary? so we don't come wondering later. might require re-deriving why we had to add this.

blueprint-bot · 2017-07-25T20:59:17Z

Address nits and comments

Preview: documentation
Coverage: core | datetime

blueprint-bot · 2017-07-25T21:08:32Z

Address nits and comments

Preview: documentation
Coverage: core | datetime

adidahiya · 2017-07-26T16:04:29Z

Am I supposed to be able to scroll the background when a dialog is open like this?

llorca · 2017-07-26T16:47:51Z

@adidahiya I suppose you opened the dialog and then hovered the primary button to view the tooltip? In that case, yes it's expected. Separate bug, there's a fix in review now: #1369

…-flex

giladgray · 2017-07-26T18:19:19Z

@adidahiya @llorca: just merged master with fix for background scrolling. hopefully that'll work as expected now. edit: confirmed works

@adidahiya please continue reviewing for potential workspace breaks!

blueprint-bot · 2017-07-26T18:24:38Z

Merge branch 'master' of github.com:palantir/blueprint into al/dialog-flex

Preview: documentation
Coverage: core | datetime

…-flex

blueprint-bot · 2017-08-10T00:27:52Z

revert padding/margin changes for legacy compat

Preview: documentation
Coverage: core | datetime

giladgray · 2017-08-10T00:36:53Z

@adidahiya tested with multiple different versions of blueprint CSS and fixed a few loose ends. the new styles now include a small patch to reset position on new Dialogs that use the .pt-dialog-container element. i also reverted changes we made to padding/margin on Dialog inner elements cuz those def don't play nice with old versions. looking good now!

blueprint-bot · 2017-08-10T00:38:24Z

actually hold up we can let the old version provide the old transitions

Preview: documentation
Coverage: core | datetime

…r scrolling requires re-implementing canOutsideClickClose but it's just dead simple fixes #1313

llorca · 2017-08-10T01:15:16Z

4e5e9c9 🏆 🥇

blueprint-bot · 2017-08-10T01:15:40Z

replace pointer-events with user-select on dialog to support container scrolling

Preview: documentation
Coverage: core | datetime

…n style

blueprint-bot · 2017-08-10T01:43:03Z

add .pt-dialog-container to markup example, remove custom docs section style

Preview: documentation
Coverage: core | datetime

adidahiya · 2017-08-10T02:57:32Z

packages/core/src/components/dialog/dialog.tsx

+
+    private handleContainerClick = (evt: React.MouseEvent<HTMLDivElement>) => {
+        // quick re-implementation of canOutsideClickClose because .pt-dialog-container covers the backdrop
+        if (this.props.canOutsideClickClose && evt.currentTarget.closest(`.${Classes.DIALOG}`) == null) {


I think the second part of the boolean condition here will always be true because currentTarget is .pt-dialog-container... I think you want evt.target instead

~~currentTarget should be container. i'm testing that the click did not come from within the .pt-dialog, meaning it came from the .pt-dialog-container itself.~~

you're completely right, i blew it ❌

blueprint-bot · 2017-08-10T03:13:28Z

fix isClickOutsideDialog logic

Preview: documentation
Coverage: core | datetime

adidahiya

why mousedown instead of click?

also you have a merge conflict

giladgray · 2017-08-10T17:51:44Z

it's always been mousedown for "outside click." there was an old bug with click where you could move the mouse around while holding it down and do weird things.

llorca · 2017-08-10T17:52:05Z

@adidahiya the real backdrop in Overlay uses onMouseDown so we're just replicating https://github.com/palantir/blueprint/blob/master/packages/core/src/components/overlay/overlay.tsx#L239

blueprint-bot · 2017-08-10T17:58:21Z

Merge branch 'master' into al/dialog-flex

Preview: documentation
Coverage: core | datetime

kmwhelan93 · 2018-05-15T15:15:59Z

packages/core/src/components/dialog/_dialog.scss

+  // LEGACY: override old (<= 1.24.0) dialog styles when inside a container to respect flex layout
+  .pt-dialog {
+    // stylelint-disable-next-line declaration-no-important
+    position: static !important;


Hey can this be reasonably removed? This is some pretty wicked styling to overcome if you don't want your dialog to be in the center of the screen.

@kmwhelan93 please file a new issue to discuss this. nearly lost this comment to the sands of time. also note that we're working on 3.0 on develop, while this PR is way back in the 1.x days.

@kmwhelan93 or even better, hit me with a PR and a passing build! ✅

Antoine Llorca added 6 commits July 20, 2017 23:11

Experimental support for both absolute and flex positioning

90ba2d9

Tweak positioning for scroll vs static

acde538

fix ContextMenu close events by using rAF to delay hide() only

9e36ec9

revert changes to Overlay, fix test, use rAF for popover focus logic

⭐ rewrite Overlay CSS positioning logic for maximum effect!

6869e76

update other component styles for latest Overlay

4f23a5d

mostly reducing duplication!

Tweak Overlay docs

fba0623

llorca requested review from cmslewis and giladgray July 21, 2017 22:14

Fix Sass linting

8551eb4

llorca force-pushed the al/dialog-flex branch from 26c49c1 to 8551eb4 Compare July 21, 2017 22:57

cmslewis reviewed Jul 24, 2017

View reviewed changes

cmslewis approved these changes Jul 24, 2017

View reviewed changes

Disable text selection on backdrop

9eb12a6

Even out toast margin / toast container padding

6719282

giladgray suggested changes Jul 25, 2017

View reviewed changes

Antoine Llorca added 2 commits July 25, 2017 13:24

Merge remote-tracking branch 'origin/master' into al/dialog-flex

3c23126

Address nits and comments

586a46f

giladgray requested a review from adidahiya July 25, 2017 20:49

Merge branch 'master' of github.com:palantir/blueprint into al/dialog…

20fcb67

…-flex

giladgray mentioned this pull request Jul 31, 2017

Insecure themadcreator/circle-github-bot#11

Open

Gilad Gray added 3 commits August 9, 2017 16:13

Merge branch 'master' of github.com:palantir/blueprint into al/dialog…

101be2d

…-flex

restore support for old container-less Dialog position+transition

bfe1502

revert padding/margin changes for legacy compat

2afcef1

actually hold up we can let the old version provide the old transitions

51bcd24

giladgray approved these changes Aug 10, 2017

View reviewed changes

giladgray mentioned this pull request Aug 10, 2017

[Dialog] scrolling UX is bad because dialog is not scrolling when hovered overlay #1313

Closed

replace pointer-events with user-select on dialog to support containe…

4e5e9c9

…r scrolling requires re-implementing canOutsideClickClose but it's just dead simple fixes #1313

add .pt-dialog-container to markup example, remove custom docs sectio…

6e0177a

…n style

giladgray force-pushed the al/dialog-flex branch from 99aa5c6 to 6e0177a Compare August 10, 2017 01:36

adidahiya approved these changes Aug 10, 2017

View reviewed changes

fix isClickOutsideDialog logic

2951594

adidahiya reviewed Aug 10, 2017

View reviewed changes

Merge branch 'master' into al/dialog-flex

79d8b67

giladgray merged commit f79d601 into master Aug 10, 2017

giladgray deleted the al/dialog-flex branch August 10, 2017 18:11

llorca mentioned this pull request Aug 15, 2017

Prepare release v1.25.0 #1448

Merged

giladgray mentioned this pull request Dec 21, 2017

Dialog position is fixed in middle screen and cannot be set using some style attributes #1957

Closed

kmwhelan93 reviewed May 15, 2018

View reviewed changes

[Overlay] New Overlay CSS plays nice with centered, scrollable dialogs #1373

[Overlay] New Overlay CSS plays nice with centered, scrollable dialogs #1373

Conversation

llorca commented Jul 21, 2017 • edited by giladgray Loading

Fixes #552, Fixes #1240, Fixes #1313

Checklist

Reviewers should focus on:

What happened

blueprint-bot commented Jul 21, 2017

Fix Sass linting

cmslewis commented Jul 24, 2017

cmslewis left a comment

Choose a reason for hiding this comment

llorca commented Jul 24, 2017

blueprint-bot commented Jul 24, 2017

Disable text selection on backdrop

llorca commented Jul 24, 2017

blueprint-bot commented Jul 24, 2017

Even out toast margin / toast container padding

giladgray left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueprint-bot commented Jul 25, 2017

Address nits and comments

blueprint-bot commented Jul 25, 2017

Address nits and comments

adidahiya commented Jul 26, 2017

llorca commented Jul 26, 2017

giladgray commented Jul 26, 2017 • edited Loading

blueprint-bot commented Jul 26, 2017

Merge branch 'master' of github.com:palantir/blueprint into al/dialog-flex

blueprint-bot commented Aug 10, 2017

revert padding/margin changes for legacy compat

giladgray commented Aug 10, 2017

blueprint-bot commented Aug 10, 2017

actually hold up we can let the old version provide the old transitions

llorca commented Aug 10, 2017

blueprint-bot commented Aug 10, 2017

replace pointer-events with user-select on dialog to support container scrolling

blueprint-bot commented Aug 10, 2017

add .pt-dialog-container to markup example, remove custom docs section style

Choose a reason for hiding this comment

giladgray Aug 10, 2017 • edited Loading

Choose a reason for hiding this comment

blueprint-bot commented Aug 10, 2017

fix isClickOutsideDialog logic

adidahiya left a comment

Choose a reason for hiding this comment

giladgray commented Aug 10, 2017

llorca commented Aug 10, 2017

blueprint-bot commented Aug 10, 2017

Merge branch 'master' into al/dialog-flex

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llorca commented Jul 21, 2017 •

edited by giladgray

Loading

giladgray commented Jul 26, 2017 •

edited

Loading

giladgray Aug 10, 2017 •

edited

Loading