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

Fonts Library: collection ID is undefined #58075

Closed
afercia opened this issue Jan 22, 2024 · 5 comments
Closed

Fonts Library: collection ID is undefined #58075

afercia opened this issue Jan 22, 2024 · 5 comments
Labels
[Feature] Font Library [Feature] Typography Font and typography-related issues and PRs [Type] Regression Related to a regression in the latest release

Comments

@afercia
Copy link
Contributor

afercia commented Jan 22, 2024

Description

This appeaers to be a regression in trunk after #57735

#57735 changed the font collection id to slug, the changes were made mainly on the PHP side. However, it appears a few parts in the JavaScript side sill expect a colelction id that it may be now undefined.

Cc @matiasbenedetto

For example:

In the odal dialog markup the ID is still used for the google fonts tab and tabpanel, but it is now undefined:

Screenshot 2024-01-22 at 15 06 32

The id to be used in these components is expected to be default-font-collection.

More importantly:
I couldn't get the prompt to allow to install Google fonts to shohw. Only after reverting #57735 I was finally able to see the prompt. There is code in the JS implementation that still expects the default-font-collection ID while now it receives undefined.

Screenshot of the prompt to allow to install fonts from Google:

Screenshot 2024-01-22 at 15 00 20

Step-by-step reproduction instructions

  • Clear your browser local storage.
  • Go to Site editor > Styles > Typography
  • Open the fonts modal dialog by clicking the 'Aa' (sigh) icon button.
  • Observe the title of the third tab within the modal dialog is 'Google fonts'. Expected: to be 'Install fonts' and change to 'Google fonts' only after permission to install has been given by the user.
  • Observe the content of the third tab is the list of Google fonts: Expected: to be the prompt to allow installation.

Screenshots, screen recording, code snippet

No response

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@afercia afercia added [Type] Regression Related to a regression in the latest release [Feature] Typography Font and typography-related issues and PRs labels Jan 22, 2024
@afercia
Copy link
Contributor Author

afercia commented Jan 22, 2024

I think this also breaks the modal dialog tabs when additional collections are added.

@matiasbenedetto I was trying to test additional collections by installing your Modern Fonts Stacks for WordPress Font Library plugin mentioned on #54713 and I was getting an error Plugin could not be activated because it triggered a fatal error.

After changing id to slug here
https://github.com/matiasbenedetto/modern-fonts-stacks-for-wp-font-library/blob/c770b588221e52b70046e30139d3db27220a9121/index.php#L14

the plugin activates correctly and a fourth tab is shown in the modal dialog:

Screenshot 2024-01-22 at 15 34 54

but the Google fonts and the Modern Fonts Stacks tab / tabpanel use the same id tabs-0-undefined so that the UI is broken.

I also suspect the fatal error comes from the fact the expected array key slug isn't set. That likely highlights on the PHP side there should be some better check when the array key is not set to prevent errors.

@afercia
Copy link
Contributor Author

afercia commented Jan 22, 2024

i'm also getting a JS Warning: Each child in a list should have a unique "key" prop.
Suspect it's the undefined id for the Google fonts tab.

@matiasbenedetto
Copy link
Contributor

Hi @afercia 👋 thanks for reporting this. It seems like this should be fixed with this PR #57884. It was already merged to a feature branch with a larger set of changes #57688 intended to be merge soon. After merging the feature branch in trunk this problem should be fixed.

After that, I'll update this little example plugin to match the new expected shape of data introduced with #57884, #57736 and #57616

@afercia
Copy link
Contributor Author

afercia commented Jan 23, 2024

Thanks for your feedback @matiasbenedetto I see in #57884 many references to id are udpated to slug so yes that will probably fix it. Glad to close this issue after the feature branch is merged.

@mikachan
Copy link
Member

I'm going to close this as #57688 has now been merged, but please re-open if the issue happens again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Font Library [Feature] Typography Font and typography-related issues and PRs [Type] Regression Related to a regression in the latest release
Projects
None yet
Development

No branches or pull requests

4 participants