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

Jump to bottom

Font Library: Convert heading text to heading elements and group fonts as a list #58834

Merged
merged 2 commits into from
Apr 30, 2024
Merged

Font Library: Convert heading text to heading elements and group fonts as a list #58834

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fa72c95
temp
t-hamano Apr 27, 2024
31fc51c
Font Library: Convert heading text to heading elements and group font…
t-hamano Apr 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
43 changes: 31 additions & 12 deletions packages/edit-site/src/components/global-styles/font-library-modal/font-collection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,18 +332,37 @@ function FontCollection( { slug } ) {
) }

<div className="font-library-modal__fonts-grid__main">
{ items.map( ( font ) => (
<FontCard
key={ font.font_family_settings.slug }
font={ font.font_family_settings }
navigatorPath={ '/fontFamily' }
onClick={ () => {
setSelectedFont(
font.font_family_settings
);
} }
/>
) ) }
{ /*
* Disable reason: The `list` ARIA role is redundant but
* Safari+VoiceOver won't announce the list otherwise.
*/
/* eslint-disable jsx-a11y/no-redundant-roles */ }
<ul
role="list"
className="font-library-modal__fonts-list"
>
{ items.map( ( font ) => (
<li
key={
font.font_family_settings.slug
}
className="font-library-modal__fonts-list-item"
>
<FontCard
font={
font.font_family_settings
}
navigatorPath={ '/fontFamily' }
onClick={ () => {
setSelectedFont(
font.font_family_settings
);
} }
/>
</li>
) ) }
</ul>
{ /* eslint-enable jsx-a11y/no-redundant-roles */ }{ ' ' }
</div>
</NavigatorScreen>

Expand Down
136 changes: 82 additions & 54 deletions packages/edit-site/src/components/global-styles/font-library-modal/installed-fonts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,66 +126,94 @@ function InstalledFonts() {
}
>
<NavigatorScreen path="/">
{ notice && (
<>
<Spacer margin={ 1 } />
<VStack spacing="8">
{ notice && (
<Notice
status={ notice.type }
onRemove={ () => setNotice( null ) }
>
{ notice.message }
</Notice>
<Spacer margin={ 1 } />
</>
) }
{ baseCustomFonts.length > 0 && (
<>
<Text className="font-library-modal__subtitle">
{ __( 'Installed Fonts' ) }
</Text>
<Spacer margin={ 2 } />
{ baseCustomFonts.map( ( font ) => (
<FontCard
font={ font }
key={ font.slug }
navigatorPath={ '/fontFamily' }
variantsText={ getFontCardVariantsText(
font
) }
onClick={ () => {
handleSetLibraryFontSelected(
font
);
} }
/>
) ) }
<Spacer margin={ 8 } />
</>
) }

{ baseThemeFonts.length > 0 && (
<>
<Text className="font-library-modal__subtitle">
{ __( 'Theme Fonts' ) }
</Text>
<Spacer margin={ 2 } />
{ baseThemeFonts.map( ( font ) => (
<FontCard
font={ font }
key={ font.slug }
navigatorPath={ '/fontFamily' }
variantsText={ getFontCardVariantsText(
font
) }
onClick={ () => {
handleSetLibraryFontSelected(
font
);
} }
/>
) ) }
</>
) }
) }
{ baseCustomFonts.length > 0 && (
<VStack>
<h2 className="font-library-modal__fonts-title">
{ __( 'Installed Fonts' ) }
</h2>
{ /*
* Disable reason: The `list` ARIA role is redundant but
* Safari+VoiceOver won't announce the list otherwise.
*/
/* eslint-disable jsx-a11y/no-redundant-roles */ }
<ul
role="list"
className="font-library-modal__fonts-list"
>
{ baseCustomFonts.map( ( font ) => (
<li
key={ font.slug }
className="font-library-modal__fonts-list-item"
>
<FontCard
font={ font }
navigatorPath={
'/fontFamily'
}
variantsText={ getFontCardVariantsText(
font
) }
onClick={ () => {
handleSetLibraryFontSelected(
font
);
} }
/>
</li>
) ) }
</ul>
{ /* eslint-enable jsx-a11y/no-redundant-roles */ }
</VStack>
) }
{ baseThemeFonts.length > 0 && (
<VStack>
<h2 className="font-library-modal__fonts-title">
{ __( 'Theme Fonts' ) }
</h2>
{ /*
* Disable reason: The `list` ARIA role is redundant but
* Safari+VoiceOver won't announce the list otherwise.
*/
/* eslint-disable jsx-a11y/no-redundant-roles */ }
<ul
role="list"
className="font-library-modal__fonts-list"
>
{ baseThemeFonts.map( ( font ) => (
<li
key={ font.slug }
className="font-library-modal__fonts-list-item"
>
<FontCard
font={ font }
navigatorPath={
'/fontFamily'
}
variantsText={ getFontCardVariantsText(
font
) }
onClick={ () => {
handleSetLibraryFontSelected(
font
);
} }
/>
</li>
) ) }
</ul>
{ /* eslint-enable jsx-a11y/no-redundant-roles */ }
</VStack>
) }
</VStack>
</NavigatorScreen>

<NavigatorScreen path="/fontFamily">
Expand Down
17 changes: 17 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
Expand Up @@ -66,6 +66,23 @@ $footer-height: 70px;
margin-bottom: 0;
}

.font-library-modal__fonts-title {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker from me, but in general it would be nice to move away from custom CSS in favor of the base components with their default styles. I assume this would require a larger refactor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similar to these styles is the label for the BaseControl component. However, we cannot use the label as a heading element.

image

Personally, I think it's fine as is for now.

Copy link
Contributor

@jasmussen jasmussen Apr 30, 2024 •
edited

Choose a reason for hiding this comment

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

It would be nice to have a heading style for that appearance that can be used in more cases. (Agree, not a blocker.)

text-transform: uppercase;
font-size: 11px;
font-weight: 600;
margin-top: 0;
margin-bottom: 0;
}

.font-library-modal__fonts-list {
margin-top: 0;
margin-bottom: 0;
}

.font-library-modal__fonts-list-item {
margin-bottom: 0;
}

.font-library-modal__font-card {
border: 1px solid $gray-200;
width: 100%;
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/specs/site-editor/font-library.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ test.describe( 'Font Library', () => {
.click();
await expect( page.getByRole( 'dialog' ) ).toBeVisible();
await expect(
page.getByRole( 'heading', { name: 'Fonts' } )
page.getByRole( 'heading', { name: 'Fonts', exact: true } )
).toBeVisible();
} );

Expand Down