Made the access and revoking of access to a collection available to any collection #59412
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pbking
requested review from
carolinan,
matiasbenedetto and
t-hamano
and removed request for
spacedmonkey
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
pbking
added
[Feature] Font Library
[Type] Bug
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/compat/wordpress-6.5/fonts/class-wp-font-collection.php ❔ lib/compat/wordpress-6.5/fonts/class-wp-rest-font-collections-controller.php ❔ lib/compat/wordpress-6.5/fonts/fonts.php |
|
Size Change: -188 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
t-hamano
reviewed
Feb 28, 2024
There was a problem hiding this comment.
Thanks for the PR!
I don't know much about server-side implementations of the Font Librariey but I think we need to first add permission to the parameter definition of the wp_register_font_collection() function on the core side, and then also include the permission field in the endpoint response. Otherwise, if the WP_Font_Collection and WP_REST_Font_Collections_Controller classes already exist in the core, I think the class definitions on the Gutenberg plugin side will not be reflected.
Or should this PR be tested on core version 6.4? Let me know if I don't understand correctly.
Comment on lines
+81
to
+98
| const getFontCollectionPermissionFromStorage = ( collectionSlug ) => { | ||
| return ( | ||
| window.localStorage.getItem( | ||
| LOCAL_STORAGE_KEY_PREFIX + collectionSlug | ||
| ) === 'true' | ||
| ); | ||
| }; | ||
|
|
||
| const [ hasPermission, setHasPermission ] = useState( | ||
| getFontCollectionPermissionFromStorage( slug ) | ||
| ); | ||
|
|
||
| const handleConfirmPermission = () => { | ||
| // eslint-disable-next-line no-undef | ||
| window.localStorage.setItem( LOCAL_STORAGE_KEY_PREFIX + slug, 'true' ); | ||
| window.dispatchEvent( new Event( 'storage' ) ); | ||
| setHasPermission( true ); | ||
| }; |
There was a problem hiding this comment.
I think unnecessary useState should be avoided as much as possible. My guess is that an ad hoc approach could be used as shown below.
const hasPermission = window.localStorage.getItem(
LOCAL_STORAGE_KEY_PREFIX + slug
);
const handleConfirmPermission = () => {
window.localStorage.setItem( LOCAL_STORAGE_KEY_PREFIX + slug, 'true' );
window.dispatchEvent( new Event( 'storage' ) );
};
const revokeAccess = () => {
window.localStorage.setItem( LOCAL_STORAGE_KEY_PREFIX + slug, 'false' );
window.dispatchEvent( new Event( 'storage' ) );
};| }, | ||
| ] } | ||
| /> | ||
| <div className="font-library__google-fonts-confirm"> |
There was a problem hiding this comment.
This classname should be generic.
| window.localStorage.setItem( LOCAL_STORAGE_ITEM, 'false' ); | ||
| window.localStorage.setItem( LOCAL_STORAGE_KEY_PREFIX + slug, 'false' ); |
There was a problem hiding this comment.
If this PR is merged, I suspect that users who have already authorized Google Fonts will have to authorize again. Is it necessary to migrate local storage keys‽
| @@ -164,6 +164,7 @@ function gutenberg_register_font_collections() { | |||
| array( | |||
| 'name' => _x( 'Google Fonts', 'font collection name', 'gutenberg' ), | |||
| 'description' => __( 'Install from Google Fonts. Fonts are copied to and served from your site.', 'gutenberg' ), | |||
| 'permission' => __( 'To install fonts from Google you must give permission to connect directly to Google servers. The fonts you install will be downloaded from Google and stored on your site. Your site will then use these locally-hosted fonts. You can alternatively upload files directly on the Upload tab.', 'gutenberg' ), | |||
There was a problem hiding this comment.
The gutenberg_register_font_collections() function performs an early return if the same Google font collection is already registered in the core. And I don't think this permission field exists in the core yet. As a result, I think this permission field is not included in the API response. Is my understanding correct?
If so, instead of performing an early reversion, wouldn't it be necessary to unregister the font collection using the wp_unregister_font_collection() function and then always re-register the font collection on the Gutenberg plugin side?
| selectedCollection.name | ||
| ) } | ||
| </Text> | ||
| <Spacer margin={ 6 } /> |
There was a problem hiding this comment.
Not specific to this PR, but across the Font Library modal, I think it's better not to use Spacer components in this kind of implementation. This is because the component itself has zero height, its actual height is unpredictable, and the margin overlaps the elements before and after the spacer.
We should be able to achieve the same layout by wrapping the elements directly under the CardBody component using a VStack component.
It looks like another PR is being referenced that is unrelated to this PR. |
65 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Adds a 'permission' property to Font Collections. This is a text string that, when set, is presented to the user who much agree to the conditions before the Fonts from that collection can be used.
Why?
Fully fixes: #54826
(NOTE: #59205 introduced a fix specific to Google Fonts. This fix will allow ANY collection to require before use and also later be revoked.
How?
permissionstring is added to Font CollectionsIf the permission value is set then a Local Storage key matching that collection's slug must be present before a user has access to the collection's fonts. If there is no
permissionvalue set then no check for the local storage key is made.If the collection has a
permissionvalue then it is shown the 'revoke access` control (introduced specifically for Google Fonts earlier) which removes that local storage tokenTesting Instructions
Navigate to Font Library Modal and open the default Google Fonts collection.
Note that the 'require permission' panel is shown.
Click the 'Allow Access' button and note that you have access
Reload the app and return to the font collection panel, note that you still have access
select the action button and revoke access to Google Fonts
Note that the 'allow access' panel is back
Change the
permissionvalue to null (infonts.php) for testingReload the font collection panel
note that the 'allow access' panel is no longer showing and the 'revoke access' control is not available
Screenshots or screencast