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

Onboarding: Add support for RTL site launch stylesheet to ETK #47890

Merged
merged 1 commit into from
Dec 1, 2020

Conversation

p-jackson
Copy link
Member

Changes proposed in this Pull Request

  • Enqueue the RTL stylesheet when is_rtl()

Copying prior art for how this is done in the ETK plugin:

Before
(see how the margins are all strange)
image

After
(also includes the fix from #47889)
image

Testing instructions

  • yarn dev --sync and sandbox an un-launched site
  • Open the launch flow and see that the margins are all looking correct now
  • Open the network tab and see that editor-site-launch.rtl.css is being loaded and the non-rtl file isn't

@p-jackson p-jackson added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. i18n Launch labels Dec 1, 2020
@p-jackson p-jackson self-assigned this Dec 1, 2020
@matticbot
Copy link
Contributor

@p-jackson p-jackson requested review from lsl and a team December 1, 2020 04:46
@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.

@matticbot
Copy link
Contributor

Caution: This PR affects files in the Editing Toolkit Plugin on WordPress.com
Please ensure your changes work on WordPress.com before merging.

D53548-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing the Editing Toolkit Plugin for more info: PCYsg-ly5-p2

Copy link
Contributor

@lsl lsl left a comment

Choose a reason for hiding this comment

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

LGTM, do you know why "Select a domain" isn't being translated? The translation exists, I'm thinking maybe a timing issue with the initial render.

@p-jackson
Copy link
Member Author

do you know why "Select a domain" isn't being translated? The translation exists, I'm thinking maybe a timing issue with the initial render.

That's this issue #47884

@lsl But what do you mean about the translation exists? When I look up translate.wordpress.org I don't see it 😕 Have I been looking in the wrong place for ETK strings?

@p-jackson p-jackson merged commit 75293ba into trunk Dec 1, 2020
@p-jackson p-jackson deleted the add/rtl-support-to-etk-launch branch December 1, 2020 21:05
@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 Dec 1, 2020
@lsl
Copy link
Contributor

lsl commented Dec 2, 2020

Oh I checked .com, it's a string from 2016, are we getting etk translations from .org?

@p-jackson
Copy link
Member Author

@lsl yes that's my understanding (afaik, I've been working with that assumption and nothing's contradicted it yet). My guess is that makes it easiest to deliver the translations to atomic.

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.

3 participants