Font Library: fix modal width on mobile viewport #54518
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
t-hamano
added
[Type] Bug
t-hamano
requested review from
colorful-tones,
jasmussen and
matiasbenedetto
t-hamano
commented
Sep 16, 2023
| isFullScreen={ true } | ||
| isFullScreen |
|
Size Change: +14 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in b12c27f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6276313002
|
jasmussen
approved these changes
Sep 18, 2023
| &.is-full-screen { | ||
|
|
||
| @include break-medium() { | ||
| width: 65vw; |
There was a problem hiding this comment.
Can we build this into the modal component itself? Mainly, I'd love to avoid as much local software as possible.
There was a problem hiding this comment.
Thanks for the review!
Can we build this into the modal component itself?
Does this mean that the width is controlled by JavaScript rather than a stylesheet? If so, I can change the approach to something like this, for example:
function FontLibraryModal() {
const isLargeViewport = useViewportMatch( 'medium' );
return (
<Modal
style={ { width: isLargeViewport ? '65vw' : undefined } }
></Modal>
);
}Or do you mean adding width constraints to the base modal component itself?
.components-modal__frame {
&.is-full-screen {
@include break-medium() {
width: 65vw;
}
}
}There was a problem hiding this comment.
I meant the latter, that ideally we have an excellent default behavior for the modal component, and we provide overrides on a per-component basis only as an exception.
I've heard someone mention an idea to build in "small", "medium", and "large" props into the modal, so that there's some variety in responsive behaviors. But all that boils down to is to have fewer local overrides, ideally. What do you think?
There was a problem hiding this comment.
Well, that might make sense. Currently, there are only two width variations for the modal component: the default auto-width or the full-width with the isFullScreen prop opted in.
However, the Font Manager has only recently been implemented, so as the UI and UX improve, the width may change, or this override style itself may become unnecessary, or may be displayed in full screen.
Considering this, I think there is no problem in fixing the bugs and pushing forward with PR for now.
There was a problem hiding this comment.
Hey all, we've been starting work on this very feature last week — a PR is up, although I haven't had the time to take a look at it just yet.
t-hamano
changed the title
ciampo
previously requested changes
Sep 18, 2023
There was a problem hiding this comment.
Just temporarily blocking the merge of this PR based on #54518 (comment)
mikachan
added
the
Backport to Gutenberg RC
|
@matiasbenedetto what's the deadline for #54471 to be merged? |
Font Library is going live in Gutenberg 16.7 so next Monday is the deadline I guess. |
ciampo
reviewed
Sep 22, 2023
There was a problem hiding this comment.
In that case, let's go with the plan proposed by @matiasbenedetto — let's merge this PR as a quick fix and refactor it once #54471 gets merged (cc @chad1008 for visibility)
| @@ -1,4 +1,11 @@ | |||
| .font-library-modal { | |||
| &.is-full-screen { | |||
There was a problem hiding this comment.
Is there a way we can avoid using is-full-screen ? It is an internal classname used by the component and therefore it shouldn't be used by its consumers.
There was a problem hiding this comment.
This selector is required for increased CSS specificity. However, as I mentioned in this comment, we can remove this CSS by using the useViewportMatch hook instead. What do you think about the hook approach?
Or, I think it can be solved by duplicating the component selectors:
.font-library-modal {
&.font-library-modal {
@include break-medium() {
width: 65vw;
}
}
}There was a problem hiding this comment.
I'd be ok with any of the two approaches
There was a problem hiding this comment.
OK, I used the approach of nesting .font-library-modal selectors. At the same time, I added the TODO comment to remind us to refactor in the future.
ciampo
reviewed
Sep 22, 2023
t-hamano
changed the title
|
Thank you! I would like to merge this PR since I'm getting the expected results with the updated selector ( |
mikachan
pushed a commit
that referenced
this pull request
Sep 25, 2023
* Font Library: fix modal width on mobile viewport * Don't use internal selector * Fix typo
|
I just cherry-picked this PR to the release/16.7 branch to get it included in the next release: 065bccb |
mikachan
removed
the
Backport to Gutenberg RC
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Why?
I think the modal should always be displayed full width in mobile viewports.
How?
I applied the width style via the media query when the viewport width is medium or higher.