Skip to content

Font Picker: Bundle list of Google Fonts #3110

Merged
swissspidy merged 28 commits intodevelopfrom
feature/3026-google-fonts
Sep 4, 2019
Merged

Font Picker: Bundle list of Google Fonts #3110
swissspidy merged 28 commits intodevelopfrom
feature/3026-google-fonts

Conversation

@spacedmonkey
Copy link
Copy Markdown
Contributor

Fixes #3026

@googlebot googlebot added the cla: yes Signed the Google CLA label Aug 28, 2019
@spacedmonkey spacedmonkey marked this pull request as ready for review August 29, 2019 11:52
Copy link
Copy Markdown
Collaborator

@swissspidy swissspidy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far it looks good, but some tests for the new font list assembling would be good.

Also, you made a good point in #3026 (comment) about performance:

I think that we should refactor the fonts to lazy load, as I am worried about the performance of loading in 512kb into every admin request.

Worth noting that this only affects the stories editor, and not the rest of the admin. However, is there any chance we can reduce the size of the JSON file after the download? There seem to be some fields in it that we don't need that could be removed.

@spacedmonkey
Copy link
Copy Markdown
Contributor Author

Worth noting that this only affects the stories editor, and not the rest of the admin. However, is there any chance we can reduce the size of the JSON file after the download? There seem to be some fields in it that we don't need that could be removed.

In 1a17964 I wrote a script to script out unneed fields. This take the json file from 512kb -> 72kb.

@spacedmonkey
Copy link
Copy Markdown
Contributor Author

spacedmonkey commented Sep 2, 2019

So far it looks good, but some tests for the new font list assembling would be good.

@swissspidy I don't that e2e are a good idea. I believe the focus should of testing should be on #3155 as the autocomplete is the help behaviour change here. Other kinds of tests here also don't really work. Unit tests in grunt code doesn't make a lot of sense.

Did anyone have any other kind of tests in mind?

@swissspidy
Copy link
Copy Markdown
Collaborator

This take the json file from 512kb -> 72kb.

Nice!

Did anyone have any other kind of tests in mind?

Unit tests for AMP_Story_Post_Type::get_fonts() and AMP_Story_Post_Type::get_fallback() would be nice. I know we haven't had some before, but now with the added complexity I think it makes sense.

@swissspidy swissspidy added this to the v1.3 milestone Sep 2, 2019
@spacedmonkey
Copy link
Copy Markdown
Contributor Author

Unit tests for AMP_Story_Post_Type::get_fonts() and AMP_Story_Post_Type::get_fallback() would be nice. I know we haven't had some before, but now with the added complexity I think it makes sense.

@swissspidy Unit tested for these functions have been added. Through I have refactored the code to make it easier to test.

I have also updated the docs as requested.

@swissspidy swissspidy self-requested a review September 4, 2019 15:09
@swissspidy swissspidy self-assigned this Sep 4, 2019
@swissspidy swissspidy merged commit 9761103 into develop Sep 4, 2019
@swissspidy swissspidy deleted the feature/3026-google-fonts branch September 4, 2019 19:37
@spacedmonkey
Copy link
Copy Markdown
Contributor Author

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Signed the Google CLA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Font Picker: Bundle list of Google Fonts

4 participants