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

[Gutenberg 7.7.1]: Fix editor close button #40329

Merged
merged 5 commits into from
Mar 24, 2020

Conversation

razvanpapadopol
Copy link

@razvanpapadopol razvanpapadopol commented Mar 20, 2020

Changes proposed in this Pull Request

Block editor updates:

  • update function to close editor and ignore existing URL
  • update tracking
  • update e2e gutenberg lib

FSE updates:

  • remove W button
  • fix back button positioning

Testing instructions

Here is a diff for testing the wpcom-block-editor: D40709-code.

Checkout this PR locally and sync wp-block-editor widget and FSE plugin using FSE Plugin Development instructions in FG.

  • On a non-FSE site start editing a page or post.
  • Pressing on top-left W button should close the editor.
  • On a FSE enabled site, go to /block-editor/page/[EDGE_STICKERED_SITE].wordpress.com
  • There shouldn't be any W button
  • [Backward compatibility] On a site running the previous version of Gutenberg everything should look and work the same as before.

Follow up:

Fixes #40315, Fixes #40334
Part of: #40078

- update handleCloseEditor fn
- update tracking
- update e2e gutenberg lib
@razvanpapadopol razvanpapadopol added [Status] Blocked / Hold [Status] Needs e2e Testing Gutenberg Edge Run e2e tests with a special site that is running the latest Gutenberg plugin snapshot labels Mar 20, 2020
@razvanpapadopol razvanpapadopol requested review from a team as code owners March 20, 2020 15:04
@razvanpapadopol razvanpapadopol self-assigned this Mar 20, 2020
@matticbot
Copy link
Contributor

@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 has changes that must be merged to WordPress.com
Hello razvanpapadopol! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D40617-code before merging this PR. Thank you!

@ramonjd ramonjd mentioned this pull request Mar 20, 2020
23 tasks
@razvanpapadopol razvanpapadopol changed the title [Gutenberg 7.7.1]: Fix block-editor close button [Gutenberg 7.7.1]: Fix editor close button Mar 20, 2020
@razvanpapadopol razvanpapadopol added [Goal] Full Site Editing [Goal] Gutenberg Working towards full integration with Gutenberg labels Mar 20, 2020
@razvanpapadopol razvanpapadopol added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Mar 20, 2020
@razvanpapadopol razvanpapadopol requested a review from simison March 20, 2020 17:25
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.

LGTM! :shipit:

@vindl
Copy link
Member

vindl commented Mar 20, 2020

Just a note that we'll have to be careful with deployment order here. Block editor diff should probably land around the same time we land Gutenberg plugin update, and at that time we should also sync FSE new plugin version containing these changes.

@simison
Copy link
Member

simison commented Mar 20, 2020

Remember that Atomic users can have older Gutenberg version so we should support both for a while.

I think there is a window global for the version?

@noahtallen
Copy link
Contributor

wpcomGutenberg.pluginVersion should work. Only in the iFrame, I think.

@noahtallen
Copy link
Contributor

I think instead of detecting the version, we could just keep both .css selectors instead of removing the old .css selectors.

@noahtallen
Copy link
Contributor

If we merge the PR with support for 7.3, then it can be deployed before releasing 7.7.1 (which is ideal, as we don't want the back button to stop working for some gap period of time)

Copy link
Contributor

@noahtallen noahtallen left a comment

Choose a reason for hiding this comment

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

I "audited" the different classnames to see which ones should work for WP 5.4 AND Gutenberg 7.7.1+, and which ones are fine to drop compatibility with WP 5.4.

It might cause some visual inconsistency in dotcom-FSE if you've turned of the Gutenberg plugin. But honestly, the plugin is designed to only work with the latest version, and there aren't a large enough number of people using it in the first place. So the subset that could see weird visual things is pretty small (i.e. Atomic user who has kept their original theme and has turned off the Gutenberg plugin).

I did test that this fixes the FSE back button issues on 7.7.1, but I didn't get a chance to test the wpcom fixes. They look good to me though :)

@razvanpapadopol razvanpapadopol added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Mar 23, 2020
…lugin

- Add legacy selector for close editor button
@razvanpapadopol razvanpapadopol added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress [Status] Blocked / Hold labels Mar 23, 2020
Copy link
Contributor

@noahtallen noahtallen left a comment

Choose a reason for hiding this comment

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

I did a lot of testing for this PR, and it works fantastically. It can be shipped immediately to simple sites and to FSE :shipit:

By the way, I also created this diff for the wpcom block editor changes. That can be done at any time now that this works for the old and new versions: D40709-code.

dotcom-fse

With Gutenberg edge:
✅ SPT navigation on new pages
✅ navigating back from new/existing pages
✅ navigating to and back from template parts like the header/footer
✅ back from new and existing posts

Without Gutenberg edge:
✅ SPT navigation on new pages
✅ navigating back from new/existing pages
✅ navigating to and back from template parts like the header/footer
✅ back from new and existing posts

non-fse/normal sites

With Gutenberg edge:
✅ SPT navigation on new pages
✅ navigating back from new/existing pages
✅ back from new and existing posts

Without Gutenberg edge:
✅ SPT navigation on new pages
✅ navigating back from new/existing pages
✅ back from new and existing posts

@razvanpapadopol razvanpapadopol added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Needs e2e Testing Gutenberg Edge Run e2e tests with a special site that is running the latest Gutenberg plugin snapshot labels Mar 24, 2020
@razvanpapadopol razvanpapadopol merged commit 50caae4 into master Mar 24, 2020
@razvanpapadopol razvanpapadopol deleted the fix/gutenberg-7.7-w-back-button branch March 24, 2020 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Goal] Full Site Editing [Goal] Gutenberg Working towards full integration with Gutenberg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dotcom FSE: remove close button on Gutenberg 7.7 Gutenberg 7.7.1 update on wpcom: fix back button URL
5 participants