Modal: add size prop to support a selection of preset modal sizes
#54471
Conversation
|
Size Change: +156 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
It would be good to get some input from @WordPress/gutenberg-design :
- is the proposed approach compatible with your needs (ie. setting a max width) ?
- do you have any advice on the number of set widths (for now we added 3 options, small / medium / large) and the corresponding px widths (300/600/900) ?
- do you have any advice on what is the expected behaviour of the component:
- when the viewport's width is lower than the selected max width
- when the
isFullScreenprop istrue(which makes the modal go full screen on mobile screens)
That seems reasonable to me, we might want to consider a min-width too.
Perhaps we'll want to align these with general content widths that emerge out of #53322. For now using values from the 4px baseline is probably adequate. Maybe:
I may be missing something but I don't think I'd expect any special behavior in this case? The modal would just fill the width (accounting any margin or padding on the container). We might consider positioning modals towards the bottom of the viewport on mobile widths, similar to sheets on mobile OS's e.g.: 
I acknowledge that's probably an optimisation to explore separately.
This doesn't only affect mobile, it also stretches the modal to fill the viewport on desktop, right? Should this be deprecated in favor of a "fill" |
|
Just a note. Is it intentional that command palette width is omitted from this, or is the idea that it conforms to one of these sizes? It sits at 420px right now. If the latter, 512px seems quite wide for the palette imo. The larger it is, the more it blocks the context of where you are/what you're doing. 
|
|
+1 on eventually standardising width values. One tiny detail. I wonder if shorter |
Sounds good — I was indeed wondering if there should be some margin around the modal to that some portion of the backdrop is always revealed
FWIW, I've also seen examples where a Modal becomes a "bottom drawer" on mobile screens. But that's a separate conversation.
I must have gotten confused by the storybook example), where we the arbitrary I also agree that a better solution could be to deprecate the
I'll let the rest of the design team answer this, but let's keep in mind that the aim of this new prop is to give a few presets for the
I don't have a strong opinion on this, but we should mention that historically components in this package used the full word when referring to sizing (ie. |
|
Thank you, everyone, for the input! I've pushed some changes to incorporate the above ideas. Summarizing the new bits:
|
chad1008
added
[Type] Enhancement
|
I wonder if medium should be the default instead of a dedicated size in the scale? Ideally consumers are creating modals that are consistent with those in core 99% of the time, and custom sizes are the exception. The current setup might promote custom sizing more than we like? |
| &.has-width-small { | ||
| max-width: 384px; | ||
| } | ||
| &.has-width-medium { | ||
| max-width: 512px; | ||
| } | ||
| &.has-width-large { | ||
| max-width: 832px; | ||
| } |
There was a problem hiding this comment.
Should these values be abstracted into variables? It may be useful for container widths (see #53322).
There was a problem hiding this comment.
Should these values be abstracted into variables?
If we want these values to be shared across the whole project (not just modal / components), we could for now add the to the base styles package (knowing that they could be later be moved to whatever the centralised place for style variables is going to be)
There was a problem hiding this comment.
As I reviewed this PR, I had a thought.
I'm not sure that it makes sense to replace isFullScreen with contentWidth="fill". That's because making the modal full screen doesn't only affect the width, but also the height of the component — and therefore using a prop called contentWidth doesn't seem appropriate.
Also, I don't really like the content part either — after all, we're setting the size of the whole modal, not just its contents.
Therefore, I propose two alternatives:
- we keep the
isFullScreenprop around (no deprecation), and we add a new prop (contentWidth, or a better name if we can find one). When theisFullScreenprop is set, the new prop gets ignored. - we find a new name for the
contentWidthprop (one that doesn't only refer to the width, and that doesn't only refer to the modal contents), so that we can effectively have it replace theisFullScreenprop
What do folks think?
| &.has-width-small { | ||
| max-width: 384px; | ||
| } | ||
| &.has-width-medium { | ||
| max-width: 512px; | ||
| } | ||
| &.has-width-large { | ||
| max-width: 832px; | ||
| } |
There was a problem hiding this comment.
Should these values be abstracted into variables?
If we want these values to be shared across the whole project (not just modal / components), we could for now add the to the base styles package (knowing that they could be later be moved to whatever the centralised place for style variables is going to be)
|
Thanks @jameskoster and @ciampo. After those helpful comments I'd propose the following:
This way we have only one prop handling the sizing of the modal, and it has a name more accurate name. Thoughts? |
|
I think the thing to avoid is contradictory configurations like size=small & isFullScreen, so that seems reasonable to me. If medium becomes the default, do we still need a medium option, or would it be cleaner to omit that? |
Setting a preset size as default will potentially visually break existing usages of The safer approach would be to leave the default value as |
|
That's true, but isn't part of this effort to establish more consistent modal usage? At the moment there is a tendency to apply custom widths to each modal ad hoc, and without a more opinionated default I'd be a little anxious about that continuing. FWIW I wasn't suggesting automatically setting the So if a modal is already applying a custom size, it shouldn't be affected. But any new instances would effectively be |
|
I believe that, in order to ensure ease of composition, each component should only take ownership of its bounding box, ie. it should not set its own size and margins. And therefore, if I were to build this component from scratch, I wouldn't add any width/height in its default state, and grow as its content requires it. But I understand that the component already currently comes with a set
Having an additional property to pick a preset size from is a convenient way for folks to pick a size without much effort (compared to writing custom styles), and in a way that is consistent across the product.
I see what you mean. While this isn't as disruptive as adding |
I had the exact same thought, but @ciampo pointed out in a separate convo that setting a default in the CSS will constrain the Granted, the current I've applied a medium default in the CSS for now, but if we decide against it, or if we feel like I've also abstracted the widths to variables in |
chad1008
force-pushed
the
add/modal-content-width
branch
from
878af2b
to
d81abb6
Compare
It's hard to say without knowing how much disruption will be caused. Maybe we play it safe and leave the current max-width (sorry @chad1008 🙈). Like you said the new prop will hopefully encourage more responsible usage.
I was thinking more generic than that; |
Alright, so to recap:
I guess this conversation can happen in parallel from the changes in this PR. We can always refactor code internal to |
chad1008
force-pushed
the
add/modal-content-width
branch
from
d81abb6
to
b76076a
Compare
This reverts commit ee89944.
Co-authored-by: Marco Ciampini <[email protected]>
chad1008
force-pushed
the
add/modal-content-width
branch
from
ed24a22
to
14669de
Compare
|
Flaky tests detected in 14669de. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6356464987
|
ciampo
added
the
Needs Dev Note
mirka
changed the title
contentWidth prop to support a selection of preset modal sizessize prop to support a selection of preset modal sizes
|
@chad1008 I see Marco added a "Needs Dev Note" label, but I'm not sure what the intent was. I'm guessing it was just to inform consumers of this new |
|
@mirka Yes, the endgame behavior of the new |
mirka
removed
the
Needs Dev Note
What?
Implements a new prop for
Modalthat constrains the content width to one of a selection of preset sizes.Why?
Consumers were finding they often needed to manually set a max width on the child elements they provided to
Modalto prevent them from pushingModal's size to the full with of the viewport.How?
The optional new prop accepts values of
small,medium, andlarge. If provided, the width of theModal's contents will be limited to300px,600px, or900px, respectively. (Note: these are just my initial values, and can most certainly be changed as needed when this PR is reviewed).The value provided to the prop is used to add an additional class name to the relevant DOM element, but this class is only added if
isFullscreenisfalse, so full screen modals will not be affected. Further, the styles implementing this change are limited to larger screens, so the automatically full screenModalrendering on mobile devices should not be affected either.dropping a ping for some folks who may want to weigh in on any design considerations for this one: @jordesign @koster @SaxonF
Testing Instructions
npm run storybook:devModal's newWith contentWidth: smallstory and test thecontentWidthcontrol.isFullScreenbehaves as expected with the new prop appliedTesting Instructions for Keyboard
n/a