Fix font library modal dialog translatable strings #58256
Merged
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
afercia
added
[Type] Bug
|
Size Change: -45 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
Jan 25, 2024
packages/editor/src/utils/l10n.js
Outdated
| */ | ||
| import { __ } from '@wordpress/i18n'; | ||
|
|
||
| export const l10n = { |
There was a problem hiding this comment.
Can you share more about this? Why is this in the editor package? Can we instead just use the string where we need it?
There was a problem hiding this comment.
I'm guessing the goal is to have a central place for these localized things.
I feel like "editor" is not the right package for that. Maybe a dedicated package like @wordpress/locale or something.
Also, since it's only used in one place now, maybe it's fine to start adhoc.
There was a problem hiding this comment.
I think this is probably aiming to implement a function equivalent to WP Core's wp_get_list_item_separator() function, as mentioned in this comment. Personally, I think adhoc is fine for now.
There was a problem hiding this comment.
Can you share more about this? Why is this in the editor package? Can we instead just use the string where we need it?
@youknowriad I tried to explain that in the PR summary:
Introduces a l10n object in the editor/utils package. For now, it only contains a listItemSeparator string. In the future this could be expanded with more strings, reshaped, or even moved to a separate package if desired. Pleaae let me know if the current approach is acceptable
I don't think we should fix it on a 'per case' basis as there are other occurrences. I created #58265
Glad to move to a new package if desired. I think in the future there would be the need for more l10n things but for now it's just this tiny string...
There was a problem hiding this comment.
For this particular case, I think it will make the code simpler if we use just the string .tff, .otf, .woff, .woff2. The loop to iterate over the file extensions is not necessary and it seems like an overkill because that info is unlikely to changes for a good amount of time.
Comment on lines
+61
to
+69
| sprintf( | ||
| /* translators: %d: Number of font variants. */ | ||
| _n( | ||
| '%d variant', | ||
| '%d variants', | ||
| variantsCount | ||
| ), | ||
| variantsCount | ||
| ) } |
There was a problem hiding this comment.
Nice addition, thanks.
packages/editor/src/utils/l10n.js
Outdated
| */ | ||
| import { __ } from '@wordpress/i18n'; | ||
|
|
||
| export const l10n = { |
There was a problem hiding this comment.
For this particular case, I think it will make the code simpler if we use just the string .tff, .otf, .woff, .woff2. The loop to iterate over the file extensions is not necessary and it seems like an overkill because that info is unlikely to changes for a good amount of time.
Comment on lines
35
to
39
| const { listItemSeparator } = l10n; | ||
|
|
||
| const supportedFormats = ALLOWED_FILE_EXTENSIONS.map( | ||
| ( extension ) => `.${ extension }` | ||
| ).join( listItemSeparator ); |
There was a problem hiding this comment.
This seems like an overkill because this info is unlikely to change.
There was a problem hiding this comment.
Glad to simplify all of that and just provide a string, agree it's a bit overkill.
matiasbenedetto
approved these changes
Jan 26, 2024
|
Thanks for the review. |
afercia
force-pushed
the
fix/font-library-modal-l10n
branch
from
954e571
to
496fcd5
Compare
|
Rebased after changes from #58180 |
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.
Fixes #58201
What?
A couple translatable strings are not translatable as they don't follow the WordPress localization best practices and are unnecessarily comples.
Why?
Following the WordPress localization best practices makes sure strings are translatable to all languages. For example:
sprintgand placeholder appropriately.,must be translatable as well.How?
sprintfwhere appropriate.Removes a__( 'and' )translatable string anc concatenation.Introduces al10nobject in theeditor/utilspackage. For now, it only contains alistItemSeparatorstring. In the future this could be expanded with more strings, reshaped, or even moved to a separate package if desired. Pleaae let me know if the current approach is acceptable Cc @youknowriad @SergeyBiryukovNote: in Core, the list item separator comes from
WP_Locale->get_list_item_separator(). It is in a centralized location and can be reused across the codebase. I'd think something similar is worth introducing also in Gutenberg.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast