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.

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

#56734 When there is no font, the border should not appear. Display further guidance text. #56825

Merged
merged 5 commits into from
Jan 11, 2024

Conversation

inc2734
Copy link
Contributor

@inc2734 inc2734 commented Dec 6, 2023

What?

#56734 When there is no font, the border should not appear. Display further guidance text.

Why?

Unnatural border have been displayed.

How?

Ensure that borders are not displayed.
If custom fonts and theme fonts are missing, a message is displayed.

Testing Instructions

  1. Open site editor.
  2. Open global style panel.
  3. Open typography panel.
  4. Remove font familes.

Testing Instructions for Keyboard

Screenshots or screencast

Before
image

After
image

@t-hamano t-hamano added [Type] Bug An existing feature does not function as intended [Feature] Typography Font and typography-related issues and PRs labels Dec 6, 2023
@apeatling apeatling added the Needs Design Feedback Needs general design feedback. label Dec 7, 2023
@t-hamano
Copy link
Contributor

This PR fixes #54527.

@t-hamano t-hamano linked an issue Dec 27, 2023 that may be closed by this pull request
Copy link
Contributor

@matiasbenedetto matiasbenedetto left a comment

Choose a reason for hiding this comment

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

Hi @inc2734, thanks for the PR.
Technically, it looks good to me 👍

I want to confirm with @jasmussen about the text displayed when no fonts are activated.

@jasmussen
Copy link
Contributor

Thanks for the ping. We can simplify the message a little, to just say "No fonts installed."

Add fonts from Google
@t-hamano t-hamano self-requested a review January 11, 2024 15:33
Copy link
Contributor

@t-hamano t-hamano left a comment

Choose a reason for hiding this comment

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

LGTM 🚀 As a follow-up, we may need to display a similar message in a modal dialog when there is no font.

image

@t-hamano t-hamano dismissed matiasbenedetto’s stale review January 11, 2024 15:39

This feedback has been addressed.

Copy link
Member

@mikachan mikachan left a comment

Choose a reason for hiding this comment

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

This looks good to me too, thank you!

I've added some small commits:

  • Fixed a linter error: 74209ce
  • Slightly improved the readability of checking if there are fonts available: fa96f0f
@t-hamano
Copy link
Contributor

The readability improvements made in fa96f0f also make sense to me 👍

@mikachan mikachan removed the Needs Design Feedback Needs general design feedback. label Jan 11, 2024
@t-hamano t-hamano merged commit ea90b75 into WordPress:trunk Jan 11, 2024
55 checks passed
@github-actions github-actions bot added this to the Gutenberg 17.6 milestone Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Typography Font and typography-related issues and PRs [Type] Bug An existing feature does not function as intended
6 participants