Font-Library Modal: implement new size prop with a fixed height
#55130
Conversation
|
Warning: Type of PR label error To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. |
|
Size Change: +3 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
chad1008
added
[Package] Components
|
Flaky tests detected in d061326. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6432648071
|
| &.font-library-modal { | ||
|
|
||
| @include break-medium() { | ||
| width: 65vw; | ||
| height: calc(100% - #{ $grid-unit-50 * 2 }); /* match full-screen `Modal` height */ |
There was a problem hiding this comment.
I may be missing nuance, but do we need height and max-height props if we use a size prop? Or did the other discussion end with the size collapsing the height by default? It seems like we might want to follow that up with further improvements, e.g. if you choose the small size, height collapses, if you choose medium or large, it comes with some preset default heights that you can then override further if you need to. Otherwise we've just created more technical debt here, it seems.
There was a problem hiding this comment.
This is the conclusion that we reached in #55062 (comment)
- We could work on a prop which toggles the origin positioning (as suggested by rich )
- Separately, we could investigate adding some form of height constraint (as discussed by chad )
One thing I'm going to say is that we're about to start working on a new version of the
Modalcomponent, powered by a third party library (likelyariakit) instead of our current custom first-party implementation. I think it may be smart to punt any of the changes discussed in this issue so that we can apply them directly to the new version of the component?
So, basically, we still need to agree on the exact behavior that preset sizes should have when it comes to the Modal's height — and we hypothesised doing so after a new version of the Modal component is released in the upcoming months.
Otherwise we've just created more technical debt here, it seems.
If you think that the changes proposed in this PR are making the situation worse, I'm happy to hold off and keep things as they are on trunk.
It seems like we might want to follow that up with further improvements, e.g. if you choose the small size, height collapses, if you choose medium or large, it comes with some preset default heights that you can then override further if you need to
This is definitely a possible approach.
More in general, I'd like to reflect on the fact that the more constraints we add to our size presets the least versatile those presets will be, adapting to fewer usecases (a good example is how the previous modal's was in-between the medium and large size, meaning that we'd need to use the large size)
There was a problem hiding this comment.
I don't think things are becoming worse here, apologies if my somewhat terse statement sounded negative, on the contrary I appreciate attention to detail.
I mainly want to push on what we're building. The way I see it, the modal should come with some excellent defaults:
- If you add no size prop, the modal has a certain size and behavior that is useful in most cases. Perhaps that's the "medium" prop if none is specified.
- Small, medium, and large props can then add additional defaults to that behavior.
- If neither of those props are sufficient, you can add css on top of that to have a hyper-local customization.
But in most cases, IMO, you should not need any local CSS at all, and the benefit would be consistent and easily recognizable modals across.
I was not able to follow the conversation on that other PR in depth, I understand the height issue was brought up mainly for the confirm component is that right? I think we can still serve a height by default and address that very valid concern:
- We could build in heights in medium and large, but keep the "small" prop without height, so it collapses.
- We could unset the default small height in the confirm component — that's a separate wrapper component, right?
We can even add a height prop to the modal component itself, to avoid the local CSS. What do you think? I think we should always strive to not need local CSS overrides, especially in cases like these.
There was a problem hiding this comment.
- If you add no size prop, the modal has a certain size and behavior that is useful in most cases. Perhaps that's the "medium" prop if none is specified.
The problem with this is that it's likely going to visually break all existing usages of Modal that assume that the component is just as large as its contents need to be.
- Small, medium, and large props can then add additional defaults to that behavior.
- If neither of those props are sufficient, you can add css on top of that to have a hyper-local customization.
This sounds good to me 👌
I was not able to follow the conversation on that other PR in depth, I understand the height issue was brought up mainly for the confirm component is that right?
I'm not sure about this, maybe @chad1008 can confirm?
If you're referring to ConfirmDialog, it is indeed a separate component from Modal, and from a quick look I couldn't see any height being currently set.
We could build in heights in medium and large, but keep the "small" prop without height, so it collapses.
This sounds like a sensible approach. We could debate whether we would like to set a fixed height, or a min height
We can even add a height prop to the modal component itself, to avoid the local CSS. What do you think? I think we should always strive to not need local CSS overrides, especially in cases like these.
I'm personally always skeptical about React props mirroring CSS props:
- they make the component's API larger
- they are more limited than CSS (for example, they don't support media queries)
- if a consumer was really keep on not writing separate CSS code, they could always pass it via the
styleprop
There was a problem hiding this comment.
I was not able to follow the conversation on that other PR in depth, I understand the height issue was brought up mainly for the confirm component is that right?
I'm not sure about this, maybe @chad1008 can confirm?
At that point in the conversation we were focused on aspect-ratio as a possible solution. My understanding was that ConfirmDialog was only being brought up as an example of an implementation that might suffer from aspect-ratio or any other approach that effectively enforced a min-height that was taller than what that implementation currently renders at.
If we were to leave the default behavior alone (ie the Modal will just be as big as it needs to be), and only apply height values to some/all of our new presets, that concern should be mitigated... the smaller implementations like ConfirmDialog aren't using a preset size so they wouldn't be impacted.
Just to clarify, when writing this PR I recognized it might very well be a temporary fix to the previous solution. We're still undecided on the final answer to the preset height question. If we wind up implementing one in the new version of Modal, we can update this and remove the CSS. If we don't, I'll just remove the TODO in the styles and leave the code as is 🙂
There was a problem hiding this comment.
Good point, Chad.
I personally think that the changes in this PR are an improvement, since we are use the new size preset and adding an additional custom height constraint.
So, I propose we merge this PR as-is and we continue the conversation on Modal separately.
There was a problem hiding this comment.
Let's wait for @jasmussen to take a look at this to make sure we're aligned :)
There was a problem hiding this comment.
There may be a brewing conflict in #55525 (comment) so worth double checking.
But just to reiterate the high level concept I would think valuable for the modal component:
- It comes with a few preset sizes.
- Some, probably most of the larger sizes, include a height. Perhaps all except the small "none"?
- In general we optimize for there to be zero need to have any local CSS. But when necessary, it should of course be okay to customize the dimensions.
There was a problem hiding this comment.
There may be a brewing conflict in #55525 (comment) so worth double checking
Thanks for flagging. There's definitely a lot going on in there, and I think it would be best to slow down a moment and understand step by step what changes we want to make to the Modal component (as an isolated, independent component)
There was a problem hiding this comment.
@ciampo are we still good to merge this for now? We talked about merging it above, if that's still good on your end, can I trouble you for a ✅ ? 😄
EDIT: My tab for this PR hadn't refreshed for some reason so I didn't see the most recent replies about slowing down re: the potentially brewing conflict until just now.
chad1008
force-pushed
the
fix/font-library-modal-sizing
branch
from
d061326 to
abbf27e
Compare
Related:
ModalHeight Controls #55062What?
Implement
Modal's newsizeprop to replace the current temporary fix.Why?
Before the
sizeprop was introduced, theisFullScreenprop was used to make the Font LibraryModaltall enough to comfortably hold all three of its internal tabs, while the width was fixed at65vwvia CSS. This was an effective fix but the new prop makes things a little more clear.How?
Setting the
Modaltosize="large"instead of using the full-screen view eliminates the need for a fixed width. It does require a fixed height instead, which was discussed in #55062.I chose the
largesize because I felt like it balanced well across the Font Library's different tabs.mediumlooked a little better for the "Library" tab, but left the other two tabs feeling a bit cramped.Future updates to
Modalmay introduce new props and/or height constraints that will make this custom style unneeded, at which point we'll revisit/remove this style. If those aren't added, this will likely be the recommended way to manageModalheights going forward, and we'll just remove thetodocomment.Testing Instructions
Modalis full height and has thehas-size-largeclass appliedModalis full screenModalheight to change.Testing Instructions for Keyboard
n/a
Screenshots or screencast
Before:
After:
