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.
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
Font Library #5285
Changes from 39 commits
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
There are no files selected for viewing
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.
how do a unregister a font collection.
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.
That's not currently possible but that work is already started: WordPress/gutenberg#54701
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.
There is now covered by
https://github.com/WordPress/gutenberg/blob/066f8777b113ca8a71c79deb052d1deb23fe9860/lib/compat/wordpress-6.5/fonts/fonts.php#L147-L149
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.
@matiasbenedetto Please add different error codes here. It will help in debugging and unit tests.
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.
Seralization can be expensive compute wise. Was json_decode considered?
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.
Also, this looks like a very JS-esque kinda way to make an array unique. Nested foreach loops or something would be more performant. Or if fonts are actual class instances instead of arrays,
array_unique()
+__toString()
could be used.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.
If this is JSON data, just serializing like this is incorrect since it doesn't follow JSON semantics regarding key order. We have
rest_stabilize_value
in Core for that.Is this function called in a hot-path? I.e. does it happen on most page loads? Or is this something called during the ingestion(?) of a font, i.e. once. If it is in a hot-path I agree this should be done more performantly. If this only happens during the initial download, then I think serializing is fine, that's how we validate uniqueness in the REST API, see
rest_validate_array_contains_unique_items
.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 is called only when a new font is installed and NOT on every page load.
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 see no references to
serialize
in the current Gutenbergtrunk
files for the Font Library feature.https://github.com/WordPress/gutenberg/tree/trunk/lib/compat/wordpress-6.5/fonts
Some "merging" happens here but the code has evolved significantly.