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: fix modal width on mobile viewport #54518

Merged
merged 4 commits into from
Sep 22, 2023
Merged

Font Library: fix modal width on mobile viewport #54518

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2911061
Font Library: fix modal width on mobile viewport
t-hamano Sep 16, 2023
028789d
Merge branch 'trunk' into fix/font-library-modal-width
t-hamano Sep 22, 2023
1dc95d5
Don't use internal selector
t-hamano Sep 22, 2023
b12c27f
Fix typo
t-hamano Sep 22, 2023
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
3 changes: 1 addition & 2 deletions 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 @@ -43,9 +43,8 @@ function FontLibraryModal( {
<Modal
title={ __( 'Fonts' ) }
onRequestClose={ onRequestClose }
isFullScreen={ true }
isFullScreen
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Minor improvement

className="font-library-modal"
style={ { width: '65vw' } }
>
<TabPanel
className="font-library-modal__tab-panel"
Expand Down
10 changes: 10 additions & 0 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,4 +1,14 @@
.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.
&.font-library-modal {

@include break-medium() {
width: 65vw;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we build this into the modal component itself? Mainly, I'd love to avoid as much local software as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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;
		}
	}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@t-hamano , I wonder if you'd like to hold on with the work on this PR, in case #54471 lands and we could use that instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ciampo Thank you for teaching me that. I think #54471 is exactly what this PR needs, so I'm happy to hold this PR .

}
}

.components-modal__header {
border-bottom: none;
}
Expand Down
Loading