-
Notifications
You must be signed in to change notification settings - Fork 57
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
Automatically add font license info for Google fonts #298
Conversation
Handle remove as well as add functionality
commit 0374c69 Merge: 83de623 072da09 Author: Sarah Norris <sarah@sekai.co.uk> Date: Tue Apr 4 16:15:50 2023 +0100 Merge pull request #297 from WordPress/add/font-family-labels Add labels around Google font family checkbox controls commit 83de623 Merge: 58491a7 b814f29 Author: Sarah Norris <sarah@sekai.co.uk> Date: Tue Apr 4 16:13:48 2023 +0100 Merge pull request #296 from WordPress/try/add-current-wp-version Add current WordPress version to style.css and readme.txt commit 58491a7 Author: github-actions <github-actions@github.com> Date: Mon Apr 3 20:54:08 2023 +0000 Version bump & changelog update commit 3a58a30 Merge: bb36a3a fd779d3 Author: Jeff Ong <jonger4@gmail.com> Date: Mon Apr 3 11:27:38 2023 -0400 Merge pull request #292 from WordPress/try/site-editor-overwrite Add Export (Clone) to site editor commit fd779d3 Author: Jeff Ong <jonger4@gmail.com> Date: Mon Apr 3 11:26:11 2023 -0400 Carry forward previous theme tags. commit 072da09 Author: Sarah Norris <sarah@sekai.co.uk> Date: Sat Apr 1 21:39:54 2023 +0100 Add checkbox labels to font family table header commit 54318c6 Author: Sarah Norris <sarah@sekai.co.uk> Date: Sat Apr 1 21:39:22 2023 +0100 Add checkbox labels to font family variants commit b814f29 Author: Sarah Norris <sarah@sekai.co.uk> Date: Sat Apr 1 20:54:04 2023 +0100 Add current WP version to readme.txt commit f4070b2 Author: Sarah Norris <sarah@sekai.co.uk> Date: Sat Apr 1 20:53:50 2023 +0100 Add current WP version to style.css commit bb36a3a Merge: b5a0a0a 8c4098a Author: Matias Benedetto <matias.benedetto@gmail.com> Date: Fri Mar 31 17:35:57 2023 -0300 Merge pull request #293 from WordPress/update/google-fonts-json-_4537839996 Update Google Fonts JSON data from API commit cd5f862 Author: Matias Benedetto <matias.benedetto@gmail.com> Date: Fri Mar 31 17:22:36 2023 -0300 setting the filename of the exported zip file commit b5a0a0a Merge: e3f9193 260d115 Author: Matias Benedetto <matias.benedetto@gmail.com> Date: Fri Mar 31 13:05:52 2023 -0300 Merge pull request #291 from WordPress/fix/280 Fixing image downloading not working in some cases commit 1b19d29 Author: Jeff Ong <jonger4@gmail.com> Date: Thu Mar 30 12:24:35 2023 -0400 Make linter happy. commit b86a1f1 Author: Jeff Ong <jonger4@gmail.com> Date: Thu Mar 30 12:23:42 2023 -0400 Rename file. commit 6efb440 Author: Jeff Ong <jonger4@gmail.com> Date: Wed Mar 29 17:27:42 2023 -0400 Add validation to form button, improve spacing. commit b5c6a17 Author: Jeff Ong <jonger4@gmail.com> Date: Wed Mar 29 17:11:01 2023 -0400 Async/await, update icon, button. commit 8bc38ef Author: Jeff Ong <jonger4@gmail.com> Date: Wed Mar 29 16:40:00 2023 -0400 Use tt3 as placeholder Co-authored-by: Ben Dwyer <ben@scruffian.com> commit 16f3c5c Author: Matias Benedetto <matias.benedetto@gmail.com> Date: Wed Mar 29 15:50:17 2023 -0300 Implementing WordPress apiFetch replacing the browser's fetch function commit 4870efb Author: Jeff Ong <jonger4@gmail.com> Date: Mon Mar 27 17:47:44 2023 -0400 Lint php. commit ebdf773 Author: Jeff Ong <jonger4@gmail.com> Date: Mon Mar 27 17:47:22 2023 -0400 Site editor rest API export. commit a7b2de4 Author: Jeff Ong <jonger4@gmail.com> Date: Mon Mar 27 13:49:35 2023 -0400 Add panel to sidebar in site editor. commit 8c4098a Author: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Date: Tue Mar 28 00:15:24 2023 +0000 Updating file commit 260d115 Author: Matias Benedetto <matias.benedetto@gmail.com> Date: Mon Mar 27 15:48:44 2023 -0300 fixing image downloading not working in some cases
This reverts commit 8d05847.
This reverts commit a4397de.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! Very useful enhancement! 😍
My main concern is this: We already have a client-side library to read the font metadata. Do you think it is worth bundling another entire library for the backend?
I don't have an answer to that question, but maybe this could help to decide:
Would the use of just the client-side library to extract the metadata increase or decrease code complexity?
When I add a font, 3 deprecation notices are displayed:
|
Not all the fonts available on Google Fonts are licensed under SIL license, there are a few licensed under Apache or Ubuntu licenses. If I add for example the font called Arimo, it adds a SIL license notice when Arimo uses Apache licensing. https://fonts.google.com/attribution Example:
|
Should we add the copyright notice and the author's name to be compliant with the attribution part of the SIL license? I guess that it won't hurt and we can we that info from the file metadata. In this way, we could make this work for local font files too. Apparently, we need to add the copyright notice too: https://themes.trac.wordpress.org/ticket/136638#comment:2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update gitignore
to exclude includes
dir?
I'd rather not include a whole additional library that does the same thing, so I'd like to try and use the client-side library here instead. Hopefully it doesn't increase the code complexity too much. |
This was the main reason I added the FontLib library 😅 Hmm, I was testing this using the Aclonica font, and the Apache license was coming through correctly from the metadata. Maybe this information isn't always correct with Google fonts... I'll do some more testing around this. Maybe it'll work better with the other library. |
Thank you @matiasbenedetto @madhusudhand for the reviews! I've updated the PR with the following:
I think this covers the feedback so far. I prefer using a single font library! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for testing again!
Ah, no! I've not seen that before. I'll look into how we can handle that. |
I think I've improved this with a bit of a refactor.
|
I haven't read the code yet, but can it be a good idea to limit it to one line instead of counting the characters? |
I considered this too, and couldn't decide which one was best! I've updated it so it will now limit the content to the first line, and still limit it to 200 characters as well in case there's one long single-line string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a little problem: let's say a family has several font faces; if you delete just one font face, but the rest of the family is there, the license is gone from readme.txt
. Apart from that this PR seems almost ready to go!
Github video upload is not working for me today so I upload the screencast of featuring the issue here:
https://i.imgur.com/nNjO55W.mp4
Good spot, @matiasbenedetto! I've fixed this by moving the 'remove' functionality to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to be working great now. Thanks for this enhancement. Very useful.
As a follow up, I created an issue to extend this functionality to the local font file assets uploaded: #348
LGTM 🚀
This adds a function that automatically handles adding the font license information for each Google font to the theme's readme.txt file.
Part of #195.