Make Dialog positioning more robust #1404

vintik · 2018-06-02T00:35:20Z

This pull request is meant to make dialog positioning more robust and move responsibility closer to the Dialog component instead of having the parent/consumer be responsible for the dialog logic.

In my eyes, the responsibility of the parent should be only what gets rendered in the Dialog. Not how the dialog renders. It can use props to change that on a case by case basis. Currently the coupling between parent and Dialog is very tight and needs to be loosened.

Our internal utility Dialog component now receives a hasNubbin which will add a nubbin. The placement of the nubbin will be determined from the overall align. Nubbins can only be added onto position="absolute" or position="overflowBoundaryElement"

Take a look at any of the alignments for PopoverTooltips:
https://design-system-react-components.herokuapp.com/?selectedKind=SLDSPopoverTooltip&selectedStory=Alignment%20%28Button%29&full=0&addons=0&stories=1&panelRight=0&addonPanel=storybook%2Factions%2Factions-panel

Big things to figure out:

Getting snapshot tests to pass
Due to reasons for trapping focus, the slds-popover class is required to be a child of Dialog and not on dialog, otherwise it breaks. I'd much rather prefer to pass these higher level classes to dialog and not have special divs that try to control the internals of Dialog.
After these changes the nubbin always points at the center of the element instead of being on the side of the element. Unless we require the user to explicitly provide the exact nubbin position, I think this is the much more reasonable default because it works consistently in many more cases. Otherwise, what would a top left nubbin position on a top left align mean? The nubbin would point up even though the reference element is below the dialog.
In a few of the parents I change the wrapper from display: inline to display: inline-block. Can it be done? Is this a breaking change? We want the "container" to take up the full height in whatever the user renders it in. Otherwise the nubbin will always point at the wrong part.
preventOverflow. Pretty sure this has never worked properly, but if we add offsets this may not work or the addition of offsets may need to move to a different time in the popper cycle. Either way I think this part would be out of scope unless this actually works for anybody currently using Dialog.

Pull Request Review checklist (do not remove)

vintik · 2018-06-04T19:48:27Z

components/menu-dropdown/__docs__/storybook-stories.jsx

@@ -12,6 +12,7 @@ import { DropdownNubbinPositions } from '../../menu-dropdown/menu-dropdown';
 import List from '../../utilities/menu-list';
 import Button from '../../button';
 import Trigger from '../../menu-dropdown/button-trigger';
+import Popover from '../../popover';


Ignore this file. Just making more complex test cases.

vintik · 2018-06-04T19:50:47Z

components/utilities/dialog/__docs__/storybook-stories.jsx

@@ -0,0 +1,31 @@
+// TODO - we should make stories to see everything on one page


Ignore this. Was looking into having a story of all things that use dialog to see all cases on one page, but didn't have the time for this.

vintik · 2018-06-04T19:51:00Z

components/utilities/dialog/index.jsx


 import { DIALOG } from '../../../utilities/constants';

 // #### Dialog doesn't pass down <IconSettings> context so repassing it here.
 import IconSettings from '../../icon-settings';
+// import { , getNubbinClassName } from '../../utilities/dialog-helpers';


Will remove

vintik · 2018-06-04T19:54:20Z

components/utilities/dialog/index.jsx

 			this.handleOpen();
 		}
 	},

 	componentDidUpdate (prevProps, prevState) {
-		if (
-			this.state.triggerPopperJS === true &&


Ignore these formatting changes, i'll change back. I can't read the code as linted. It hurts my eyes, but I'll change back before committing.

vintik · 2018-06-04T19:54:47Z

components/utilities/dialog/index.jsx

-		return { ...popperData.style, left, top, position };
+		const propOffsets = this.getPropOffsetsInPixels(this.props.offset);
+
+		// FIXME before merge - gotta rename from margin to offset


Got to address fixme and rename to getNubbinOffsets

vintik · 2018-06-04T19:55:25Z

components/utilities/dialog/index.jsx

+
+		if (this.props.position === 'absolute' || this.props.position === 'overflowBoundaryElement') {
+			Object.assign(style, {
+				outline: 0,


I don't think the outline: 0 is actually needed. Will remove.

vintik · 2018-06-04T20:13:15Z

components/utilities/dialog/index.jsx

 				style={style}
 				onKeyDown={this.handleKeyDown}
 				onMouseEnter={this.props.onMouseEnter}
 				onMouseLeave={this.props.onMouseLeave}
 				{...this.props.containerProps}
 				ref={this.setDialogContent}
+				role={this.props.variant}


Not sure how to address this properly and whether this can be done, but seemed sane to me that the role would be based off the variant, or if we want we can have some sort of mapping, but it's basically the same thing with different labels.

This is fine. We may want to do an enum object though incase we add more variants in the future.

Yeah was my thinking, but only if the direct variant -> role mapping ends up insufficient.

vintik · 2018-06-04T20:14:47Z

components/popover/popover.jsx

@@ -478,7 +475,6 @@ const Popover = createReactClass({
 					aria-describedby={`${this.getId()}-dialog-body`}
 					className={classNames(
 						'slds-popover',


This is incredibly annoying since it can't be moved up to the Dialog classnames level or at least I'm not sure how to go about it since it's difficult to follow all the logic of trapping the focus in the popover and following this up and down through the components, life cycle methods, and refs.

I think it's easier to follow from a comparing the HTML nodes here to SLDS if you pass in the whole popover HTML in as a child.

For now leaving it as is, but after thinking about this some more, I think that I disagree, the Dialog is the Popover. You should be able to render an empty popover. Why should it be div div.popover #contents when it can just be div.popover #contents. It's a bit annoying because we're trying to unify all these components under the concept of a "Dialog" but SLDS limits us in this fashion. Leaving it as is, because I'm already rushed for time as is.

interactivellama · 2018-06-07T06:46:18Z

The output definitly looks nicer:
. From an architecture, we should be as "smart" as we can as long as folks can override things to be flexible.

On the subject of trapping focus, the first tabbable node in the dialog contents should receive focus. I made the dialog div get focus, because I knew it would always exist and I wouldn't need to do a re-render to focus via component ref. I'm open to changing, but it's tricky because there's a mix of DOM query and React rendering.

I think it's reasonable to ask folks to be inline block if they want to absolutely align something to it.

I'm not sure what you mean by preventOverflow not working. If you are referring to the position flip/swap. This PR will make the nubbin swap positions if you want to incorporate it into this PR.

Something is off with the nubbin like z-index or something:

interactivellama · 2018-06-18T16:50:49Z

@vintik, @davidlygagnon will be working on Popover's nubbin flip #1211 probably next week and be in the Dialog codebase also. Just FYI.

vintik · 2018-06-18T17:00:48Z

I was thinking about adding it in this week if @davidlygagnon is okay with that. I finally have some time cleared up and we need these dialog changes merged within the next 2 weeks. About to have sprint planning. Hopefully this remains my top priority.

davidlygagnon · 2018-06-19T05:14:58Z

Sounds good @vintik . I'll take another issue that @interactivellama assigned to me. Thanks!

vintik · 2018-07-05T20:50:08Z

components/utilities/dialog/index.jsx

 			DOMElementFocus.storeActiveElement();
 			DOMElementFocus.setupScopedFocus({
-				ancestorElement: scopedElement.querySelector('.slds-popover'),


Was kind of weird that we used to do a search for slds-popover, even though slds-popover is a class we control. Now since the Dialog no longer receives a div child that it's tightly bound to, and the Dialog is of a type of popover, we can just use the dialogContent reference directly.

If we want to be a bit defensive, we can do the scan if the class isn't on dialogContent, but then there'd be a bug anyways and the tests should catch that. I don't think this is an appropriate time for defensive checks

The focusing helpers were originally taken from jQuery UI, so DOM query snuck in.

Still uses a dom reference, there's just no need to query, there's only one possible element it can be.

vintik · 2018-07-05T20:54:03Z

components/utilities/dialog/index.jsx

 						this.props.contentsClassName
 					) || undefined
 				}
+				id={this.props.id}


Should be removed. This should be passed as containerProps instead.

vintik · 2018-07-05T20:56:32Z

utilities/dialog-helpers.js

+*
+*
+*/
+// FIXME - still need to account for border shadow of 2px. probably only needs to be added to the rotated height.


Not sure this was ever accounted for previously.

vintik · 2018-07-05T21:16:38Z

Not exactly sure how I resolve the conflicts? :)

interactivellama · 2018-07-05T21:23:24Z

Just pull down master and overwrite what you have locally. These images are unrelated to what you are doing.

interactivellama · 2018-07-11T05:44:46Z

components/popover/popover.jsx

@@ -570,7 +549,7 @@ const Popover = createReactClass({

 		this.renderOverlay(this.getIsOpen());

-		const containerStyles = { display: 'inline' };
+		const containerStyles = { display: 'inline-block' }; // ATTN FIXME - can we make this change??!


I think it will be OK. slds-button should already be inline block

/** * @summary This neutralizes all the base styles making it look like a text link * * @name base * @selector .slds-button * @restrict button, a, span * @variant */ .slds-button { position: relative; display: inline-block; padding: 0;

Oh i need to remove the comment

interactivellama · 2018-07-11T05:51:01Z

components/utilities/dialog/index.jsx

@@ -372,13 +389,16 @@ const Dialog = createReactClass({
 	},

 	render () {
-		let style = {};
+		const style = {};


See master branch. This needs to let once master branch is merged in. Object.assign was removed and spread operator was put in it's place.

interactivellama · 2018-07-11T05:51:38Z

components/utilities/dialog/index.jsx


 		if (
 			this.props.position === 'absolute' ||
 			this.props.position === 'overflowBoundaryElement'
 		) {
-			style = this.getPopperStyles();
+			Object.assign(style, {


use spread operator here.

interactivellama · 2018-07-11T15:08:52Z

Tooltips are not being read on focus of trigger anymore
Your PR:
https://design-system-react-co-pr-1404.herokuapp.com/?selectedKind=SLDSPopoverTooltip&selectedStory=Base&full=0&addons=1&stories=1&panelRight=0&addonPanel=%40storybook%2Faddon-a11y%2Fpanel

Master branch:
http://design-system-react-components.herokuapp.com/?selectedKind=SLDSPopoverTooltip&selectedStory=Base&full=0&addons=1&stories=1&panelRight=0&addonPanel=storybook%2Factions%2Factions-panel

Cmd F5 will enable Voiceover on a Mac. It should read the tooltip when the button is focused. This will need to be fixed. The button and the dialog should be connected by the aria attributes which will cause the button to read the tool contents when focued.

interactivellama · 2018-07-11T16:20:45Z

We'll need to make sure we mention the initial offset orientation change in the release notes. A checkProps would be nice, but it wouldn't go away after the offset numbers were updated.

…pper

vintik · 2018-07-23T22:52:05Z

@interactivellama So finally got time again :) lol it's been a crazy couple of months.

Looked into it and realize now why tooltip trap focus doesn't work. It was never supposed to work in the first place and according to the original design would probably be a bug. Frankly, have no idea how it's trapping focus on master branch.

https://github.com/salesforce/design-system-react/blob/master/components/utilities/dialog/index.jsx#L170

If we want I can switch the variant from tooltip to popover and it should work as expected.

interactivellama · 2018-08-07T06:26:15Z

Fixes #1211

salesforce-cla bot added the cla:signed label Jun 2, 2018

vintik commented Jun 4, 2018

View reviewed changes

vintik force-pushed the vintik/dialogs branch from 3f53fb8 to 8e6f401 Compare June 15, 2018 00:26

interactivellama mentioned this pull request Jul 5, 2018

Progress Indicator Style/Color Updates #1374

Closed

vintik commented Jul 5, 2018

View reviewed changes

interactivellama temporarily deployed to design-system-react-co-pr-1404 July 11, 2018 05:39 Inactive

interactivellama reviewed Jul 11, 2018

View reviewed changes

interactivellama temporarily deployed to design-system-react-co-pr-1404 July 11, 2018 20:46 Inactive

vintik added 3 commits July 11, 2018 13:59

WIP

717a564

WIP - building more test cases and trying to understand how to fix po…

3e965b8

…pper

trying to consolidate all dialogs to have the offset margin hardcode

379be5b

vintik added 7 commits July 11, 2018 14:26

linter is stupid, but managed to 'autofix' a bunch of stuff luckily

599785d

tests suck

c80479f

Fix stuff

25e693e

i'll never understand the linter because it wont tell me whats wrong

af6f7eb

update snapshots? why do they all update?

ba7a82d

Remove custom storybook of Victors

593efcb

some cleanup

039a8ac

vintik force-pushed the vintik/dialogs branch from 9131119 to 039a8ac Compare July 23, 2018 22:41

Support flipping of dialogs

da24687

interactivellama temporarily deployed to design-system-react-co-pr-1404 July 25, 2018 18:44 Inactive

Merge branch 'master' into vintik/dialogs

efda782

interactivellama temporarily deployed to design-system-react-co-pr-1404 July 25, 2018 18:46 Inactive

Add a first version of release notes for dialogs changes

3e943b9

interactivellama temporarily deployed to design-system-react-co-pr-1404 July 25, 2018 19:19 Inactive

This was referenced Jul 25, 2018

Allow nubbin to change position when popover flips #1304

Closed

Popover looks wrong when changing position #1211

Closed

autofix prettier issues

f49b858

interactivellama temporarily deployed to design-system-react-co-pr-1404 July 25, 2018 20:05 Inactive

Deprecate offset prop for dialog components

a75ee19

interactivellama temporarily deployed to design-system-react-co-pr-1404 July 25, 2018 20:37 Inactive

Cleanup release notes

2c9d891

interactivellama temporarily deployed to design-system-react-co-pr-1404 July 25, 2018 20:38 Inactive

More lint issues and potentially fixes the voiceover tooltip issue

e6e2344

interactivellama temporarily deployed to design-system-react-co-pr-1404 July 25, 2018 20:55 Inactive

more lint fix

4b78eb3

interactivellama deployed to design-system-react-co-pr-1404 July 25, 2018 21:03 View deployment

interactivellama merged commit 86e9c88 into salesforce:master Aug 3, 2018

interactivellama mentioned this pull request Aug 3, 2018

Fix Info Icon Tooltip Position #1448

Closed

		@@ -0,0 +1,31 @@
		// TODO - we should make stories to see everything on one page

Make Dialog positioning more robust #1404

Make Dialog positioning more robust #1404

Uh oh!

Conversation

vintik commented Jun 2, 2018

Pull Request Review checklist (do not remove)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vintik Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

interactivellama Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vintik Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vintik Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

interactivellama Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

interactivellama commented Jun 7, 2018

Uh oh!

interactivellama commented Jun 18, 2018

Uh oh!

vintik commented Jun 18, 2018

Uh oh!

davidlygagnon commented Jun 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vintik commented Jul 5, 2018

Uh oh!

interactivellama commented Jul 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

interactivellama Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

interactivellama Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

interactivellama commented Jul 11, 2018

Uh oh!

interactivellama commented Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

vintik Jun 4, 2018 •

edited

Loading

interactivellama Jun 7, 2018 •

edited

Loading

vintik Jun 7, 2018 •

edited

Loading

vintik Jun 4, 2018 •

edited

Loading

interactivellama Jun 7, 2018 •

edited

Loading

interactivellama commented Jul 5, 2018 •

edited

Loading

interactivellama Jul 11, 2018 •

edited

Loading

interactivellama Jul 11, 2018 •

edited

Loading

interactivellama commented Jul 11, 2018 •

edited

Loading

vintik commented Jul 23, 2018 •

edited

Loading