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

Fix Global Styles on wpcom #40690

Merged
merged 2 commits into from
Apr 2, 2020
Merged

Fix Global Styles on wpcom #40690

merged 2 commits into from
Apr 2, 2020

Conversation

noahtallen
Copy link
Contributor

Changes proposed in this Pull Request

#40489 broke global styles inside the iFrame (changing a font did not change the font inside the preview). This fixes that.

The root cause is that the <style /> element we dynamically update for Global Styles was not appended at the end of the other <style /> elements. I'm not quite sure why, but it seems that this script can begin executing before all the style elements have been loaded. So the fix is to wait some time to add the style element. I chose the isEditorReady selector for this, but there could be a better heuristic. At any rate, this fixes the issue for me.

Testing instructions

  1. Sync to dotcom. Changing the font selection in Global Styles should update the editor.
  2. Test the same on a self hosted site
  3. Test that changes in Global Styles: fix previews not showing global styles #40489 still work correctly

@noahtallen noahtallen added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Type] Defect [Team] Cylon labels Apr 2, 2020
@noahtallen noahtallen requested a review from a team as a code owner April 2, 2020 02:26
@noahtallen noahtallen self-assigned this Apr 2, 2020
@matticbot
Copy link
Contributor

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello noahtallen! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D41256-code before merging this PR. Thank you!

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@oandregal
Copy link
Contributor

Squeezing a quick pass-by comment in case it's any helpful: code looks fine to me. Thanks for the quick turn-around @noahtallen

Copy link
Member

@vindl vindl left a comment

Choose a reason for hiding this comment

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

Works well for me. :shipit:

@noahtallen noahtallen merged commit 86269c5 into master Apr 2, 2020
@noahtallen noahtallen deleted the fix/global-styles branch April 2, 2020 19:48
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants