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

Editing Toolkit: Update to 2.8.5 #47442

Merged
merged 4 commits into from Nov 17, 2020
Merged

Editing Toolkit: Update to 2.8.5 #47442

merged 4 commits into from Nov 17, 2020

Conversation

p-jackson
Copy link
Member

@p-jackson p-jackson commented Nov 15, 2020

Changes proposed in this Pull Request

  • Version bump and changelog

Changes to Editing Toolkit since 2.8.4:

Testing instructions

  1. Load the diff (D52773-code) on your sandbox and confirm that the above changes work correctly
  2. Test that the new version of the plugin doesn't break Atomic sites by following the instructions in the "Atomic Testing" section at PCYsg-ly5-p2.

@p-jackson p-jackson added the Editing Toolkit For issues and PRs that affect the Editing Toolkit plugin label Nov 15, 2020
@p-jackson p-jackson self-assigned this Nov 15, 2020
@matticbot
Copy link
Contributor

@p-jackson p-jackson added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 15, 2020
@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.

D52773-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

@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.

@p-jackson
Copy link
Member Author

p-jackson commented Nov 15, 2020

Known issue with the php unit tests running in CI. They pass when run locally.

@autumnfjeld autumnfjeld self-requested a review November 16, 2020 02:16
@autumnfjeld
Copy link
Contributor

I'll start on testing this. :)

Copy link
Contributor

@autumnfjeld autumnfjeld left a comment

Choose a reason for hiding this comment

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

@p-jackson Per Atomic testing instructions I have checked that the Editor loads with this ETK build, both with and without Gutenberg plugin. Success! ✅

@andrewserong
Copy link
Member

andrewserong commented Nov 16, 2020

@p-jackson @apeatling while testing this release I can see a visual issue with the Payments block, which I think could be related to #47245 (probably not caused by the PR, but revealed by it, if that makes sense!) — Update: actually it's probably not that PR, I just noticed that it depends on Automattic/jetpack#17702 which isn't merged 🤔

With this release patched there's a stray 0 character beneath the block:

image

Without this release patched, it renders a Subscribe block, but I don't see the stray 0 character:

image

Does this happen for anyone else? (Tested on a simple site on the Free plan)

@autumnfjeld
Copy link
Contributor

autumnfjeld commented Nov 16, 2020

TEST FOR: Gutenboarding: plans grid requests data in the correct locale (#47227)

Note: my launch flow went to /start/launch-site
- ✅ View the plans grid in gutenboarding: it should be translated and the prices should be in your selected currency
- ✅ View the plans grid in the launch flow: it should be translated and the prices should be in your selected currency (my currency is USD, which is just $ no letters like AUD$)
- ✅ Open gutenboarding in incognito tab: the currency in the plans grid should be a decent guess given your IP

TEST FOR: Gutenboarding i18n: i18n for domain picker in the launch flow (#47178)**
⛔ I could not reproduce any of the tests specified in the PR description, I couldn't get the correct currency punctuation for french (the comma in the price e.g. 3,40)

@autumnfjeld
Copy link
Contributor

TEST FOR: Add block preview for Premium Content block (#47200)

For Business Plan Site:

  • ✅ Make sure that the block renders correctly in the post body

  • ✅ Verify that the "Subscribe" button for the block in the post body is a jetpack/recurring-payments block (you should be able to see this as data-type on the div in the browser inspector)

  • ⛔ Open the block list in the side panel and hover over the 'Premium Content' block in the search results --> See screenshot "No preview available"
    Screen Shot 2020-11-16 at 16 41 28

  • ✅ Verify that the block preview displays correctly --> See screenshot below
    Screen Shot 2020-11-16 at 16 50 26

For Free Site:
Block did not render (or preview?) correctly:
Screen Shot 2020-11-16 at 16 51 40

@andrewserong
Copy link
Member

andrewserong commented Nov 16, 2020

@autumnfjeld looks like the Read more about Payments... message will be removed in @apeatling's Jetpack PR Automattic/jetpack#17696 — the PR's merged, but I assume it hasn't been deployed to simple sites yet.

Update on my testing of the Premium Content block: it's working much like in Autumn's screenshots for me now, turns out I had an issue in my sandbox with my test site, so now testing on a fresh Free site it's looking a fair bit better. There is still the stray zero character rendering during the loading state, but it's appearing both with and without sandboxing this version change, so appears to be unrelated to the recent PRs in this ETK plugin release:

stray-zero-character-small

@apeatling
Copy link
Member

Starting a review of our PRs in here this morning 👍

@apeatling
Copy link
Member

Just confirming that this issue is fixed by Automattic/jetpack#17696:

Screen Shot 2020-11-16 at 9 39 15 AM

I will look at merging that change on dotcom early today because it's a rough visual issue and I'd prefer not to wait on the Jetpack release.

@apeatling
Copy link
Member

I could not reproduce the block preview missing on search, this appeared fine for me:

Screen Shot 2020-11-16 at 9 41 11 AM

@apeatling
Copy link
Member

Add block preview for Premium Content block (#47200)
Provide block context to the payments button. (#47245)

Both of these change test fine for me on simple and atomic. 👍

@jeyip
Copy link
Contributor

jeyip commented Nov 16, 2020

Known issue with the php unit tests running in CI. They pass when run locally.

If anyone is curious, here's the issue for the failing PHP unit spec #47255.

@jeyip
Copy link
Contributor

jeyip commented Nov 16, 2020

Created a diff to remove the outdated ET build D52840-code whenever this update is merged

@p-jackson
Copy link
Member Author

Rebased to include #47450

@jeyip
Copy link
Contributor

jeyip commented Nov 16, 2020

TEST FOR: use get_iso_639_locale() for coming soon page links (#47307)

Did a very basic smoke test and just double-checked that the login button works. @roo2 Would love to have you smoke test this version bump as well if possible 🙂

  • ✅ Simple site login link on a coming soon page works (Site Language EN and ES)
  • ✅ Atomic site login link on a coming soon page works (Site Language EN and ES)

TEST FOR: [eslint] Adds rule one-var and autofixes all related errors (#47310)

✅ Tested atomic and simple sites with the one editing toolkit block that was refactored (event countdown block)

@jeyip jeyip requested review from ciampo and roo2 November 16, 2020 23:17
@p-jackson p-jackson merged commit 52a3781 into master Nov 17, 2020
@p-jackson p-jackson deleted the update/etk-2.8.5 branch November 17, 2020 03:57
@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 Nov 17, 2020
@sirreal
Copy link
Member

sirreal commented Nov 17, 2020

Deployed in r216900-wpcom

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Editing Toolkit For issues and PRs that affect the Editing Toolkit plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants