-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Font Library #5285
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
Font Library #5285
Changes from 1 commit
9a00814
6f9aefc
d78d99b
1d74144
42eeb48
9e06a93
b323b67
715eea9
e54426a
1078767
699f45b
a6dca33
9ba9187
5084180
46a1e9f
7e666e6
e6cce43
673c042
72bbec5
1d8de84
d769cc5
c4a31d2
57f05a1
8933cb8
9aeb0ba
1725825
d4bce59
89e888f
ab70608
739edeb
534264e
c2d38d6
2a715cd
6bf7be3
bbb09e9
654dd1d
635d870
8714206
2e08b53
8c5ddec
ae15e18
04fe719
839ff70
5116443
d89e201
e4b2a85
8e3e711
947e2cb
88ec9b7
3e63080
e218ec8
02d6767
641e9f3
5178b1e
4612703
f69e6e5
17c9fd3
3021090
db11c1f
31af894
21e3d33
9aa43bf
f38bb59
b005321
2b29de4
aaf3741
d6534cc
3b01b46
f16ead0
30369c6
e1f0b58
aec6c93
b34fe52
e948c1b
0926426
11b26db
feb1199
a776cee
909da30
fd706d1
7081941
b49da9a
2d369aa
86dab9e
b167d7f
1c23f09
4f7fd91
14ca3bd
8d9f496
1243b31
c5c6ec4
f398761
d85e545
07d01ff
ec52113
3967726
a19ac36
05d3ff7
963d226
e28134f
b2a96fc
640159e
d44da25
603ee9e
7382406
d9ffea9
9edfb39
c4de43f
9deeb4a
6c9e856
89cd000
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -143,13 +143,13 @@ private function get_validation_errors( $font_families, $files ) { | |||||
| $error_messages = array(); | ||||||
|
|
||||||
| if ( ! is_array( $font_families ) ) { | ||||||
| $error_messages[] = __( 'fontFamilies should be an array of font families.', 'gutenberg' ); | ||||||
| $error_messages[] = __( 'fontFamilies should be an array of font families.' ); | ||||||
| return $error_messages; | ||||||
| } | ||||||
|
|
||||||
| // Checks if there is at least one font family. | ||||||
| if ( count( $font_families ) < 1 ) { | ||||||
| $error_messages[] = __( 'fontFamilies should have at least one font family definition.', 'gutenberg' ); | ||||||
| $error_messages[] = __( 'fontFamilies should have at least one font family definition.' ); | ||||||
| return $error_messages; | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -163,7 +163,7 @@ private function get_validation_errors( $font_families, $files ) { | |||||
| ) { | ||||||
| $error_messages[] = sprintf( | ||||||
| // translators: 1: font family index. | ||||||
|
||||||
| // translators: 1: font family index. | |
| /* translators: %s: font family index. */ |
Translator comments are in the form of:
/* translators: <token || number>: Description<comma || period> */🔢 Applies elsewhere in the PR.
See the information for Plugins, the best reference I could find 😅. This is the same practice used in WordPress Core.
Outdated
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.
- This should use sprintf with placeholders for things like
fontFamily - Why square brackets instead of parentheses? That's not common AFAIK
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.
The same goes for all the other error messages, really.
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.
Why square brackets instead of parentheses? That's not common AFAIK
One place I do see brackets used is when validating an object in the request from the endpoint schema (see
wordpress-develop/src/wp-includes/rest-api.php
Line 2344 in 2ae28c5
| $is_valid = rest_validate_value_from_schema( $v, $args['properties'][ $property ], $param . '[' . $property . ']' ); |
The error messages use a string like object_name[property_name] to refer to the property being validated. We're using this format specifically for custom validation error messages in the new controller implementation, to match the error format from the default schema validation (now with appropriate sprintf usage, see WordPress/gutenberg#58675).
Outdated
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.
As noted elsewhere, params should use snack case.
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.
Other schema are cached for performance reasons. Why was this avoided here.
Outdated
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.
This should return an array of remove fonts and boolean.
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.
Updated with the changes of this PR WordPress/gutenberg#54829. The output of the install and uninstall endpoints are now standardized for success and errors.
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.
Done in 5116443 ✅
Uh oh!
There was an error while loading. Please reload this page.
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.
This should use sprintf and use a placeholder for
fontFamilies. And of course proper translator comments.