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

wpcom block editor: hide new Gutenberg preview button #40388

Merged
merged 5 commits into from
Mar 24, 2020

Conversation

simison
Copy link
Member

@simison simison commented Mar 24, 2020

Changes proposed in this Pull Request

This PR forces the mobile preview button to be the default in the block editor.

We have to do this because the device preview functionality in Gutenberg 7.7 works by overwriting stylesheets.

However overwriting stylesheets can only occur on the same domain due to cross-site security. See: p7DVsv-8gl-p2#comment-27501

Further issue tracking in #40401

Testing instructions

Check out this branch and build it:

npx lerna run build --scope='@automattic/wpcom-block-editor' -- -- --watch

Upload the files in apps/wpcom-block-editor/dist to your sandbox.

  1. YOUR_TEST_SITE_URL should be running Gutenberg Edge
  2. YOUR_TEST_SITE_URL should be sandboxed
  3. Tea should be brewed for at least three minutes. 🍵

Try creating a post:

https://[YOUR_TEST_SITE_URL]/wp-admin/post-new.php (this PR doesn't address this, needs a follow up as described in #40388 (comment))

https://wordpress.com/block-editor/post/[YOUR_TEST_SITE_URL]

Check that the preview button in the editor always defaults always to the mobile version:

Screen Shot 2020-03-24 at 10 20 05 pm

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

@roo2
Copy link
Contributor

roo2 commented Mar 24, 2020

It doesn't seem to be loading that SCSS file at all as is. I was able to test it locally and added a console.log('HERE') to widgets.wp.com/wpcom-block-editor/editor.js so I believe I was testing it correctly... But nothing in apps/wpcom-block-editor/src/wpcom/features/legacy-preview-button.scss was loaded 🤔

@ramonjd
Copy link
Member

ramonjd commented Mar 24, 2020

For me the CSS for wpcom.editor isn't being loaded. Probably not enqueued in wpcom.

I threw the CSS in apps/wpcom-block-editor/src/wpcom/features/legacy-preview-button.scss into apps/wpcom-block-editor/src/calypso/editor.scss to check and it works.

calypso.editor JS and CSS are being loaded. :)

@ramonjd
Copy link
Member

ramonjd commented Mar 24, 2020

I've tested enqueuing the files using a change in D40729-code

The patch below moves the CSS changes in this PR to /default, on the assumption that "default" means both WPCOM and Calypso.

See the README file for confirmation

Patch file:

serve-legacy-preview-button-css-from-default.txt

@simison
Copy link
Member Author

simison commented Mar 24, 2020

on the assumption that "default" means both WPCOM and Calypso.

.
└── src/
	├── default ← Always loaded.
	├── calypso ← Only loaded when you access Gutenberg through the iFrame.
	├── wpcom   ← Only loaded when you access Gutenberg on Simple and Atomic sites.

via

I think default loads also on Jetpack sites (@mmtr correct?) and we don't wanna break the feature for those. Hence wpcom seems like the way to go, sounds like it'll load also when iframing the block-editor?

@mmtr
Copy link
Member

mmtr commented Mar 24, 2020

That's correct @simison. Features in default are loaded in all sites (Simple, Atomic and Jetpack). If you want to target both Calypso and WP Admin of WP.com sites (Simple and Atomic) then wpcom would be the place.

@simison simison marked this pull request as ready for review March 24, 2020 14:40
@simison simison requested review from a team as code owners March 24, 2020 14:40
Copy link
Member Author

@simison simison left a comment

Choose a reason for hiding this comment

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

I've confirmed that this gets loaded only when iframing Gutenberg via Calypso.

Tested FSE and non-FSE sites with Gutenberg 7.7.1:

Screenshot 2020-03-24 at 17 49 03

As well non-FSE site with Gutenberg 7.3 — no affect on previous preview button:

Screenshot 2020-03-24 at 17 48 28

All these now open on both mobile and desktop widths this preview modal in Calypso, which happens to also have the mobile preview:

Screenshot 2020-03-24 at 17 24 05

@simison simison removed the request for review from a team March 24, 2020 16:13
@ramonjd ramonjd mentioned this pull request Mar 24, 2020
23 tasks
@simison
Copy link
Member Author

simison commented Mar 24, 2020

I'd like to keep this fix just for Calypso-iframed Gutenberg can call that enough for Gutenberg 7.7.

I'll do another PR to address wp-admin, but since it needs some PHP and Jetpack handling in addition to D40729-code, I'd like to look into that separately. Jetpack release is on April 7th. We should really just move whatever is in Jetpack to FSE plugin for ease. :-)

@noahtallen
Copy link
Contributor

overwriting stylesheets can only occur on the same domain due to cross-site security

🤦‍♂

@noahtallen noahtallen merged commit 7b831d2 into master Mar 24, 2020
@noahtallen noahtallen deleted the update/wpcom-gutenberg-legacy-preview branch March 24, 2020 17:55
@simison
Copy link
Member Author

simison commented Mar 24, 2020

To be clear, I'd like us to figure a solution before the next Gutenberg release. Hiding UI elements like this isn't very sustainable. 😁

Copy link
Contributor

@Addison-Stavlo Addison-Stavlo left a comment

Choose a reason for hiding this comment

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

I was just testing this on a site with dotcom FSE enabled (but not running due to theme selection).

It did change the preview from the dropdown button back to this:
Screen Shot 2020-03-24 at 1 59 31 PM

Which opens the previewer default in 'desktop' mode. There dont seem to be any issues with this, but I noticed you mentioned it should default to 'mobile' view?

Anyways, the overall change seems to be working.

@simison
Copy link
Member Author

simison commented Mar 24, 2020

Thanks for testing @Addison-Stavlo 🙌

Which opens the previewer default in 'desktop' mode. There dont seem to be any issues with this, but I noticed you mentioned it should default to 'mobile' view?

Desktop view is expected. 👍 I mean to say that mobile preview is available in that popup in Calypso, which is great because we're hiding the equavelent core feature.

@Addison-Stavlo
Copy link
Contributor

I mean to say that mobile preview is available in that popup in Calypso

Great! That was definitely present and working then. 😄

@ramonjd
Copy link
Member

ramonjd commented Mar 24, 2020

Nice work everyone! I'll retire D40729-code for (maybe) future use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants