Skip to content
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

WP.com Block Editor: Enqueue missing common styles on the front-end. #14070

Merged
merged 6 commits into from
Nov 25, 2019

Conversation

kwight
Copy link
Contributor

@kwight kwight commented Nov 18, 2019

Up to now, both the script and styles for the common module have been enqueued on the enqueue_block_editor_assets hook; this left out necessary styles for justified text from being displayed on the front-end.

This PR splits out the styles from enqueue_scripts, and enqueues them for both the editor and the front-end.

Editor
Screen Shot 2019-11-18 at 12 48 25 PM

Front-end
Screen Shot 2019-11-18 at 12 48 43 PM

Testing instructions

  • Switch to this branch.
  • Create a new post, add multiple lines of content to a Paragraph block.
  • Format the content as "Justify", and verify it formats the content properly in the editor.
  • Publish the post, and verify the text is justified on the front-end too.

Proposed changelog entry
[BUG] Fixed Justify styling for the WP.com Block Editor.

@kwight kwight requested a review from a team November 18, 2019 20:50
@kwight kwight self-assigned this Nov 18, 2019
@kwight kwight added [Pri] Normal [Type] Bug When a feature is broken and / or not performing as intended [Feature] WordPress.com Block Editor labels Nov 18, 2019
@jetpackbot
Copy link

jetpackbot commented Nov 18, 2019

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: December 3, 2019.
Scheduled code freeze: November 26, 2019

Generated by 🚫 dangerJS against f25842e

@kwight kwight added the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Nov 18, 2019
@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Nov 19, 2019
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

Would there be a way to not load this on each and every front-end page as soon as Jetpack is connected to WordPress.com? Ideally we'd only use it when the buttons are used in a post that's being displayed.

If that's not an option, maybe we should move those common styles to a stylesheet that's already loaded on each front-end page by Jetpack, such as the concatenated jetpack.css.

@gwwar
Copy link
Contributor

gwwar commented Nov 19, 2019

Would there be a way to not load this on each and every front-end page

@jeherve The current size of http://widgets.wp.com/wpcom-block-editor/common.min.css is only 670B. Can we please follow up on this in another issue? There more to think through with either alternative approach.

@kwight
Copy link
Contributor Author

kwight commented Nov 20, 2019

Would there be a way to not load this on each and every front-end page as soon as Jetpack is connected to WordPress.com?

Good point! @gwwar I can add a has_block check to keep this more tidy.

@kwight kwight force-pushed the fix/editor-common-styles branch from 10ee131 to adb3ae8 Compare November 20, 2019 01:06
@jeherve
Copy link
Member

jeherve commented Nov 20, 2019

I'm afraid I'm still getting fatal errors:

Fatal error: Uncaught Error: Call to undefined function get_current_screen() 

The current size of http://widgets.wp.com/wpcom-block-editor/common.min.css is only 670B.

I'm less worried about the file size, and more about the added CSS enqueue, especially coming from a module that can't be easily disabled by most site owners and when the enqueue is not hosted on the site so can't be handled by most concatenating / caching plugins. Some folks monitor the different enqueues on their site very closely, and will not be happy to see us adding a file without an easy way to dequeue it (short of doing it via code).

Since the file includes so little, wouldn't we better served by hosting it locally in Jetpack, and adding it to this list so it gets concatenated in jetpack.css for everyone:

* @var array The handles of styles that are concatenated into jetpack.css.

As a result there won't be any additional enqueue, and we won't have to worry that the file is enqueued on all pages since jetpack.css is already enqueued on all pages.

@kwight kwight force-pushed the fix/editor-common-styles branch from 4f2e218 to f25842e Compare November 21, 2019 21:45
@kwight
Copy link
Contributor Author

kwight commented Nov 21, 2019

@jeherve With the latest updates, it'll only be enqueued if:

  • the WordPress.com user is connected, and
  • the user is in the block editor, or
  • the front-end is displaying blocks that use the justify setting.

This way, it's only enqueued when truly needed, and we're not adding stuff to jetpack.css for those that'll never need it. Would that work for you?..

@kwight kwight added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Nov 21, 2019
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This is just what we needed, thank you. It should be good to merge!

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Nov 22, 2019
@jeherve jeherve added this to the 8.0 milestone Nov 22, 2019
@gwwar
Copy link
Contributor

gwwar commented Nov 23, 2019

❤️ Thanks @jeherve and @kwight !

@jeherve jeherve merged commit 71d45eb into master Nov 25, 2019
@jeherve jeherve deleted the fix/editor-common-styles branch November 25, 2019 09:29
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Nov 25, 2019
jeherve added a commit that referenced this pull request Nov 25, 2019
jeherve added a commit that referenced this pull request Nov 25, 2019
* 8.0 Release: running changelog

* Changelog: add #13921

* Changelog: add #13980

* Changelog: add #13905

* Changelog: add #13971

* Changelog: add #13984

* Changelog: add #14009

* Changelog: add #13620

* Remove things that will ship in 7.9.1

* Changelog: add 7.9.1 release (#14044)

* Changelog: add base for 7.9.1 release

* Update release date and post link

* Changelog: add #14066

* Update changelog for 7.9.1

* Changelog: add #13405

* Changelog: add #13841

* Changelog: add #13924

* Changelog: add #13986

* Changelog: add #14010, #14028, #14053, #14055.

* Changelog: add #14054

* Changelog: add #14031

* Changelog: add #14039

* Changelog: add #14050

* Changelog: add #14070

* Changelog: add #14082

* Changelog: add #14084

* Changelog: add #14111

* Changelog: add #13961

* Changelog: add #14047

* Changelog: add #14091

* Changelog: add #14108

* Changelog: add #14121
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] WordPress.com Block Editor [Pri] Normal [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants