`Modal`, `ConfirmModal` components and design patterns #88

lyzadanger · 2021-06-03T13:59:14Z

This (initially draft) PR contains new Modal and ConfirmModal components and their corresponding supporting design patterns and pattern-library content.

Modal wraps a Dialog and adds a background overlay and a close-on-outside-click functionality. Modal + Dialog here = the Dialog component in the client. ConfirmModal is analogous to ConfirmDialog in the client and doesn't deviate too much from it.

I've added some responsiveness to the modal positioning and sizing in this variant. See implementation and comments for details (a test to see if the source is self-documenting and commented enough).

In draft until...

Feedback gathered about general approach, design
Tests added (after that)

To try it out

Check out this branch, run make dev and visit the new Dialogs pattern library page in your browser. The modal examples are last on the page.

Screenshots

Note that the pattern-library examples include all supported Dialog bells and whistles, including an icon in the header, which may not be applicable to modals. But it's helpful to see everything in the mix...

Modal, narrow viewport (with initial-focus input):

The same Modal, wider viewport:

ConfirmModal, narrow viewport:

ConfirmModal, narrow viewport, with excessively long content (NB, this example is not included in source):

ConfirmModal, wider viewport:

Fixes #86

The newer `Example` layout for pattern-library examples is more conducive to responsive-ness than the older `PatternExamples` and `PatternExample` styling. Make it columnar on narrower screens. `Example` is still a prototype layout, but may supersede/replace the table-based `PatternExample(s)` styling in the future. The `Example` layout is used on the `Dialogs` component pattern library page, and it needs to be responsive so one can view the responsive modal patterns in a responsive setting.

src/components/Modal.js

src/pattern-library/components/patterns/DialogPatterns.js

lyzadanger · 2021-06-03T14:06:07Z

styles/mixins/patterns/_molecules.scss

+  //     of viewport.
+  // - sizing: Set a min-width to make sure the modal isn't too dinky. Set
+  //     a max-width to make sure it isn't too ginormously wide on wide screens.
+  @media screen and (min-width: 48rem) {


Bah, I had it in an earlier comment here but then deleted it and neglected to re-add: this 48rem magic is a rem-translation of the 768px tablet breakpoint from the client. I will make sure to add commenting.

robertknight · 2021-06-04T11:42:38Z

Some feedback on the pattern library unrelated to the changes in this PR: The different font sizes for the component description and example description looks a bit odd, possibly because they are visually part of the same region (that is to say, there is no border or background separating the two paragraphs):

lyzadanger · 2021-06-04T12:02:26Z

Some feedback on the pattern library unrelated to the changes in this PR: The different font sizes for the component description and example description looks a bit odd, possibly because they are visually part of the same region (that is to say, there is no border or background separating the two paragraphs):

Noted! There is work to do here, for sure.

robertknight · 2021-06-04T12:13:54Z

I noticed that the vertical positioning of modals is sensitive to the width of the dialog as well as the height. Wide viewport:

Same viewport height but narrower:

To me it would make more sense if the vertical position depended only on the height.

robertknight

Overall this looks great. I didn't have a lot of comments to add. What use cases did you have in mind for Dialogs that are not Modals?

src/components/Modal.js

src/pattern-library/components/patterns/DialogPatterns.js

src/pattern-library/components/patterns/MoleculePatterns.js

robertknight · 2021-06-04T12:24:17Z

styles/mixins/patterns/_molecules.scss

+
+  // For wider viewports:
+  // - positioning: keep horizontal centering but bring top nearer to top
+  //     of viewport.


I noted this in another comment, but I think we might want to make the vertical centering behavior dependent on the height rather than the width of the viewport.

It's good feedback. I'm working on splitting the media-query-based differentiation out into two now:

Screens that are wide enough: set a reasonable min- and max-width so that the modal isn't too dinky but also doesn't stretch way out

Screens that are tall enough: position the modal nearer to the top of the viewport

e.g. Narrow, but tall screen:

styles/mixins/patterns/_molecules.scss

lyzadanger · 2021-06-04T12:49:22Z

I didn't have a lot of comments to add. What use cases did you have in mind for Dialogs that are not Modals?

I wanted to evaluate replacing a couple of what are now Panel elements in the client with Dialog—the salient difference being that Dialog can route focus. We had an organization raise an a11y flag on a couple of the Panel elements in the client (e.g. the one that alerts you that you need to log in before you can annotate) because of focus issues.

codecov · 2021-06-04T16:20:57Z

Codecov Report

Merging #88 (6a35284) into main (27f98fa) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main       #88   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         8    +1     
  Lines          115       123    +8     
  Branches        38        39    +1     
=========================================
+ Hits           115       123    +8

Impacted Files	Coverage Δ
src/components/Modal.js	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27f98fa...6a35284. Read the comment docs.

robertknight

I had some minor queries but overall this looks good. Please have a look through and make changes as you see fit.

src/components/Modal.js

src/components/test/Modal-test.js

src/pattern-library/components/patterns/DialogPatterns.js

src/pattern-library/components/patterns/MoleculePatterns.js

styles/mixins/patterns/_molecules.scss

robertknight · 2021-06-07T17:15:37Z

styles/mixins/patterns/_molecules.scss

+
+  // This sizing applies to smaller viewports
+  width: 90vw;
+  max-width: 90vw;


I noticed a jump as the dialog shrinks in size when going from a width that is less than the min-width media query below to one that is greater.

What would your preferred behavior be? I'm never sure how best to optimize for the "but it isn't animated/smooth when resizing a screen" issue...

lyzadanger requested a review from robertknight June 3, 2021 14:00

lyzadanger commented Jun 3, 2021

View reviewed changes

src/components/Modal.js Outdated Show resolved Hide resolved

lyzadanger commented Jun 3, 2021

View reviewed changes

src/pattern-library/components/patterns/DialogPatterns.js Show resolved Hide resolved

lyzadanger commented Jun 3, 2021

View reviewed changes

src/pattern-library/components/patterns/DialogPatterns.js Show resolved Hide resolved

lyzadanger commented Jun 3, 2021

View reviewed changes

lyzadanger mentioned this pull request Jun 3, 2021

Add basic clean-theme support and update card, frame patterns #89

Merged

robertknight reviewed Jun 4, 2021

View reviewed changes

lyzadanger force-pushed the Modal-component branch from 11e77e8 to 7d6bfe7 Compare June 4, 2021 16:20

lyzadanger changed the title ~~For draft review: Modal, ConfirmModal components and design patterns~~ Modal, ConfirmModal components and design patterns Jun 4, 2021

lyzadanger marked this pull request as ready for review June 4, 2021 16:20

lyzadanger requested a review from robertknight June 4, 2021 16:21

lyzadanger mentioned this pull request Jun 4, 2021

Add prefixes to all component class names to prevent collisions #90

Merged

lyzadanger force-pushed the Modal-component branch from 7d6bfe7 to fa0841f Compare June 7, 2021 13:45

robertknight approved these changes Jun 7, 2021

View reviewed changes

lyzadanger added 2 commits June 7, 2021 13:47

Add modal (molecule) pattern

e17da41

Add Modal, ConfirmModal components

6a35284

lyzadanger force-pushed the Modal-component branch from fa0841f to 6a35284 Compare June 7, 2021 17:48

lyzadanger merged commit b394593 into main Jun 7, 2021

lyzadanger deleted the Modal-component branch June 7, 2021 17:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`Modal`, `ConfirmModal` components and design patterns #88

`Modal`, `ConfirmModal` components and design patterns #88

lyzadanger commented Jun 3, 2021 •

edited

Loading

lyzadanger Jun 3, 2021

robertknight commented Jun 4, 2021

lyzadanger commented Jun 4, 2021

robertknight commented Jun 4, 2021

robertknight left a comment

robertknight Jun 4, 2021

lyzadanger Jun 4, 2021

lyzadanger commented Jun 4, 2021 •

edited

Loading

codecov bot commented Jun 4, 2021 •

edited

Loading

robertknight left a comment

robertknight Jun 7, 2021

lyzadanger Jun 7, 2021

Modal, ConfirmModal components and design patterns #88

Modal, ConfirmModal components and design patterns #88

Conversation

lyzadanger commented Jun 3, 2021 • edited Loading

In draft until...

To try it out

Screenshots

lyzadanger Jun 3, 2021

Choose a reason for hiding this comment

robertknight commented Jun 4, 2021

lyzadanger commented Jun 4, 2021

robertknight commented Jun 4, 2021

robertknight left a comment

Choose a reason for hiding this comment

robertknight Jun 4, 2021

Choose a reason for hiding this comment

lyzadanger Jun 4, 2021

Choose a reason for hiding this comment

lyzadanger commented Jun 4, 2021 • edited Loading

codecov bot commented Jun 4, 2021 • edited Loading

Codecov Report

robertknight left a comment

Choose a reason for hiding this comment

robertknight Jun 7, 2021

Choose a reason for hiding this comment

lyzadanger Jun 7, 2021

Choose a reason for hiding this comment

`Modal`, `ConfirmModal` components and design patterns #88

`Modal`, `ConfirmModal` components and design patterns #88

lyzadanger commented Jun 3, 2021 •

edited

Loading

lyzadanger commented Jun 4, 2021 •

edited

Loading

codecov bot commented Jun 4, 2021 •

edited

Loading