(RSP-1526) Moving size prop into DialogTrigger and css fixes for fullscreen dialog #229

LFDanLu · 2020-02-27T00:00:26Z

Closes https://jira.corp.adobe.com/browse/RSP-1526

✅ Pull Request Checklist:

Included link to corresponding Issue.
Added/updated unit tests and storybook for this change (for new code or code which already has tests).
Filled out test instructions.
Updated documentation (if it already exists for this component).
Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

adobe-bot · 2020-02-27T00:05:48Z

Build successful! 🎉

adobe-bot · 2020-02-27T00:24:57Z

Build successful! 🎉

packages/@react-spectrum/dialog/stories/DialogTrigger.stories.tsx

snowystinger · 2020-02-27T20:55:00Z

packages/@react-spectrum/dialog/stories/DialogTrigger.stories.tsx

+    'type: popover, fullscreen',
+    () => render({type: 'popover', size: 'fullscreen'})
+  )
+  .add(


this one seems weird, same question

packages/@react-spectrum/dialog/stories/DialogTrigger.stories.tsx

snowystinger · 2020-02-27T20:56:38Z

packages/@react-spectrum/dialog/stories/Dialog.stories.tsx

    'with hero, isDimissable',
    () => renderHero({isDismissable: true, onDismiss: action('dismissed')})
  )
-  .add(


can we not specify these anymore? (S/M/L)

snowystinger · 2020-02-27T20:57:09Z

packages/@react-spectrum/dialog/stories/Dialog.stories.tsx

    'form',
    () => renderWithForm({})
  )
-  .add(


this needs to be added somewhere, it's a good story to demonstrate tabbing order

snowystinger · 2020-02-27T20:58:14Z

packages/@adobe/spectrum-css-temp/components/dialog/index.css

 .spectrum-Dialog--fullscreen {
-  width: calc(100vw - 32px);
-  height: calc(100vh - 32px);
+  width: calc(100vw - 64px);


where does 64 come from? should it change with scale?

32px is the padding that is supposed to be around the fullscreen dialog. Since that padding is 32px for both top/bottom and left/right, the above needed to get doubled so it actually fit the 32px padding container

Not sure if it should change with scale, didn't see fullscreen in the XD designs, I'll ask around

Updated to 40px constant as per design: https://excelerate-team.slack.com/archives/G8R3V30FL/p1582920957021300

whoops

adobe-bot · 2020-02-28T20:15:06Z

Build successful! 🎉

adobe-bot · 2020-02-28T20:35:42Z

Build successful! 🎉

adobe-bot · 2020-02-28T20:36:19Z

Build successful! 🎉

snowystinger · 2020-02-28T20:52:14Z

packages/@adobe/spectrum-css-temp/components/dialog/index.css

 .spectrum-Dialog--fullscreen {
-  width: calc(100vw - 32px);
-  height: calc(100vh - 32px);
+  width: calc(100vw - 80px);


calc 2* fullscreen margin?

…ctrum-v3 into RSP-1526

adobe-bot · 2020-02-28T21:15:22Z

Build successful! 🎉

ktabors · 2020-03-02T17:44:25Z

packages/@adobe/spectrum-css-temp/components/modal/index.css


  /* Distance between top and bottom of dialog and edge of window for fullscreen dialog */
-  --spectrum-dialog-fullscreen-margin: 32px;
+  --spectrum-dialog-fullscreen-margin: 40px;


Is there a common place for components/dialog/index.css and this file to share this variable?

Ideally I would've included it into the css var but Larry said that file was autogenerated and to put new vars at the top of the index.css file.

adobe-bot · 2020-03-02T18:35:09Z

Build successful! 🎉

codecov · 2020-03-02T18:35:15Z

Codecov Report

❗ No coverage uploaded for pull request base (master@7c16163). Click here to learn what that means.
The diff coverage is 85.71%.

@@            Coverage Diff            @@
##             master     #229   +/-   ##
=========================================
  Coverage          ?   70.58%           
=========================================
  Files             ?      206           
  Lines             ?     3977           
  Branches          ?      848           
=========================================
  Hits              ?     2807           
  Misses            ?      587           
  Partials          ?      583

Impacted Files	Coverage Δ
...kages/@react-spectrum/dialog/src/DialogTrigger.tsx	`85.18% <ø> (ø)`
packages/@react-spectrum/overlays/src/Modal.tsx	`100% <100%> (ø)`
packages/@react-spectrum/dialog/src/Dialog.tsx	`82.35% <66.66%> (ø)`

adobe-bot · 2020-03-02T23:32:14Z

Build successful! 🎉

adobe-bot · 2020-03-03T18:40:29Z

Build successful! 🎉

…rum-v3 into RSP-1526

adobe-bot · 2020-03-05T18:28:07Z

Build successful! 🎉

dannify · 2020-03-05T18:42:23Z

packages/@react-spectrum/dialog/src/DialogTrigger.tsx

    mobileType = type === 'popover' ? 'modal' : type,
    hideArrow,
    targetRef,
+    size,


Instead of adding a size prop to this, can we take advantage of type? We could have types: modal, popover, tray, fullscreen, fullscreenTakeover.
Make sense?

Can you also add those 2 new types to mobileType. We want to allow people to override some default behaviors regarding mobile views.

Ehh, I considered this before and it feels weird to me since it dilutes the intuitive meaning of the type prop. To me type indicates the kinda of shape/look of the Dialog where the size prop is its literal meaning. This would mean a user could provide a size to dialog (S/M/L) and specify type=fullscreen as well which doesn't really make sense as a pairing

If we are trying to distinguish that fullscreen/fullscreenTakeover is for modal only, we could go one step further and change size to be modalSize since it only really applies to the modal type as is.

yeah mostly the problem here is that DialogTrigger handles too much anyway. The situation you describe will exist anyway in other forms because we are mixing popover and modal behaviors. We will need in on mobileType so it seemed the best to just add it to type as well. Otherwise based on your suggestion we would need modalSize and mobileModalSize?

yup we would need to add mobileModalSize as well in case the user wants separate size behavior on mobile vs desktop.

I'll go with your suggestion for now and add the 2 fullscreen types

adobe-bot · 2020-03-05T20:09:19Z

Build successful! 🎉

adobe-bot · 2020-03-05T20:23:34Z

Build successful! 🎉

Moving size prop into DialogTrigger and css fixes for fullscreen dialog

72be25e

LFDanLu requested review from dannify, devongovett and snowystinger as code owners February 27, 2020 00:00

accidentally moved size in dialog types

098c90b

snowystinger reviewed Feb 27, 2020

View reviewed changes

MidnightCoder06 previously approved these changes Feb 27, 2020

View reviewed changes

LFDanLu added 2 commits February 28, 2020 11:45

Addressing review comments

c2748ec

fixing tests

c0e34a2

LFDanLu added 2 commits February 28, 2020 12:27

adjusting fullscreen dialog padding to 40px as per design

7fd85cd

Merge branch 'master' into RSP-1526

5a15657

snowystinger reviewed Feb 28, 2020

View reviewed changes

LFDanLu added 2 commits February 28, 2020 13:08

adding 40px dialog fullscreen var

f458ea4

Merge branch 'RSP-1526' of https://github.com/adobe-private/react-spe…

00d128e

…ctrum-v3 into RSP-1526

snowystinger previously approved these changes Feb 28, 2020

View reviewed changes

ktabors previously approved these changes Mar 2, 2020

View reviewed changes

Merge branch 'master' into RSP-1526

3bbeb8d

Removing size for popover dialogs

de82990

LFDanLu dismissed stale reviews from ktabors and snowystinger via de82990 March 2, 2020 23:22

ktabors previously approved these changes Mar 3, 2020

View reviewed changes

LFDanLu added 2 commits March 2, 2020 16:48

Merge branch 'master' into RSP-1526

2e24fb3

Merge branch 'master' into RSP-1526

ad4a3c0

Updating context in prep for Rob's changes

e6ed66f

LFDanLu dismissed ktabors’s stale review via e6ed66f March 5, 2020 18:19

Merge branch 'master' of https://github.com/adobe-private/react-spect…

33ec143

…rum-v3 into RSP-1526

dannify reviewed Mar 5, 2020

View reviewed changes

LFDanLu added 2 commits March 5, 2020 12:00

Replacing size in DialogTrigger with fullscreen (mobile)type

f21f019

forgot to remove size from dialog trigger types

cffe870

devongovett approved these changes Mar 5, 2020

View reviewed changes

devongovett merged commit c07b175 into master Mar 5, 2020

devongovett deleted the RSP-1526 branch March 5, 2020 23:01

LFDanLu mentioned this pull request Mar 5, 2020

(DNM) Making only dismissible modal dialogs closable via underlay click #256

Closed

5 tasks

(RSP-1526) Moving size prop into DialogTrigger and css fixes for fullscreen dialog #229

(RSP-1526) Moving size prop into DialogTrigger and css fixes for fullscreen dialog #229

Uh oh!

Conversation

LFDanLu commented Feb 27, 2020

✅ Pull Request Checklist:

📝 Test Instructions:

Uh oh!

adobe-bot commented Feb 27, 2020

Uh oh!

adobe-bot commented Feb 27, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

adobe-bot commented Feb 28, 2020

Uh oh!

adobe-bot commented Feb 28, 2020

Uh oh!

adobe-bot commented Feb 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adobe-bot commented Feb 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adobe-bot commented Mar 2, 2020

Uh oh!

codecov bot commented Mar 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

adobe-bot commented Mar 2, 2020

Uh oh!

adobe-bot commented Mar 3, 2020

Uh oh!

adobe-bot commented Mar 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LFDanLu Mar 5, 2020 • 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!

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!

adobe-bot commented Mar 5, 2020

Uh oh!

adobe-bot commented Mar 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

codecov bot commented Mar 2, 2020 •

edited

Loading

LFDanLu Mar 5, 2020 •

edited

Loading