Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Font-Library Modal: implement new size prop with a fixed height #55130

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open

Font-Library Modal: implement new size prop with a fixed height #55130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/edit-site/src/components/global-styles/font-library-modal/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ function FontLibraryModal( {
<Modal
title={ __( 'Fonts' ) }
onRequestClose={ onRequestClose }
isFullScreen
size="large"
className="font-library-modal"
>
<TabPanel
Expand Down
9 changes: 5 additions & 4 deletions packages/edit-site/src/components/global-styles/font-library-modal/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
.font-library-modal {
// @todo: If a new prop is added to the Modal component that constrains
// the content width, we should use that prop instead of this style.
// see https://github.com/WordPress/gutenberg/issues/54471.
// @todo: If a new prop is added to the Modal component that fixes the height
// and/or constrains the origin, we should use that instead of this style.
// see https://github.com/WordPress/gutenberg/issues/55062.
&.font-library-modal {

@include break-medium() {
width: 65vw;
height: calc(100% - #{ $grid-unit-50 * 2 }); /* match full-screen `Modal` height */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Modal component, powered by a third party library (likely ariakit) 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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.
  2. Small, medium, and large props can then add additional defaults to that behavior.
  3. 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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.

  1. Small, medium, and large props can then add additional defaults to that behavior.
  2. 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 style prop

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙂

Copy link
Contributor

@ciampo ciampo Oct 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's wait for @jasmussen to take a look at this to make sure we're aligned :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@chad1008 chad1008 Nov 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

max-height: none;
}
}

Expand Down
Loading