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 plugin: disable additional features for Jetpack sites #34476

Closed
simison opened this issue Jul 5, 2019 · 15 comments · Fixed by #38248
Closed

wpcom-block-editor plugin: disable additional features for Jetpack sites #34476

simison opened this issue Jul 5, 2019 · 15 comments · Fixed by #38248
Assignees
Labels
[Goal] Gutenberg Working towards full integration with Gutenberg Jetpack [Type] Bug When a feature is broken and / or not performing as intended

Comments

@simison
Copy link
Member

simison commented Jul 5, 2019

We currently build 2 bundles of the wpcom-block-editor app:

  • common: loaded by the block editor in any env (Simple, Atomic and Jetpack sites, both iframed and not iframed).
  • calypso: loaded by the iframed block editor in Calypso.

However, there are features in the common bundle that shouldn't be exposed to Jetpack sites, since they are intended specifically for WP.com users. Particularly, we don't have to disable the NUX tour nor unregister the experimental blocks on Jetpack sites.

One possible solution is to split the common bundle into 2 different ones.

1st bundle (it can remains as common and loaded in any environment - iframed and not iframed):

  • Rich text (justify / underline).
  • Switch to classic.
  • Tracking.

2nd bundle (it can be called wpcom, to be loaded in Simple and Atomic sites only - iframed and not iframed):

  • Disable NUX tour.
  • Unregister experimental blocks.

cc @Automattic/jetpack-crew

@simison simison added [Type] Bug When a feature is broken and / or not performing as intended Jetpack [Goal] Gutenberg Working towards full integration with Gutenberg labels Jul 5, 2019
@jeherve
Copy link
Member

jeherve commented Jul 5, 2019

Jetpack sites have additional "underline" and "strikeout" editor buttons.

I believe this was done on purpose. See Automattic/jetpack#12070

NUX flow for Gutenberg gets disabled for these sites

I am not familiar with this flow, however ¯_(ツ)_/¯

@simison
Copy link
Member Author

simison commented Jul 5, 2019

NUX flow for Gutenberg gets disabled for these sites

I am not familiar with this flow

You can create a Jurassic ninja site without jetpack and open the block editor to see the flow:

image

Jetpack sites have additional "underline" and "strikeout" editor buttons.

I believe this was done on purpose. See Automattic/jetpack#12070

Thanks! I don't get that impression from this PR tho. Testing instructions are for using Jetpack sites via Calypso iframed editor. I'd expect features to get enabled only when used via Calypso, not in self-hosted wp-admin.

The problem might also be that it's not clear that common.js is loaded for all sites, causing devs to add features there that they didn't intend to ship to self-hosted Jetpack sites. So might be that some features are meant to be shipped only on .com (like disabling Guten-NUX) and some for both? :-) Would be good to check and clarify, regardless. :-)

@kwight
Copy link
Contributor

kwight commented Jul 5, 2019

I believe this was done on purpose.

It was, yes. The idea was that all Jetpack users get the much-requested Underline and Strikethrough, and by being in WP Admin, users also get it in the iframed Calypso editor too.

So might be that some features are meant to be shipped only on .com (like disabling Guten-NUX)

That does sound like an oversight; I'd expect the core NUX to not be on dotcom, but still presented on a Jetpack site. @mmtr am I remembering that correctly, I think you worked on that?..

@simison
Copy link
Member Author

simison commented Jul 5, 2019

Could the solution be to split this list further into two bundles which another gets enqueued only when block editor is iframed and another one registers always?

import './disable-nux-tour';
import './rich-text';
import './switch-to-classic';
import './style.scss';
import './track-search';
import './unregister-experimental-blocks';

@mmtr
Copy link
Member

mmtr commented Jul 5, 2019

So might be that some features are meant to be shipped only on .com (like disabling Guten-NUX)

That does sound like an oversight; I'd expect the core NUX to not be on dotcom, but still presented on a Jetpack site. @mmtr am I remembering that correctly, I think you worked on that?..

Definitely an oversight. I can see a couple of integrations (disabling tips or unregistering experimental blocks) we shouldn't expose to Jetpack sites.

Could the solution be to split this list further into two bundles which another gets enqueued only when block editor is iframed and another one registers always?

We already have the calypso folder for that. The problem here is that we still want these integrations in the non-iframed WP Admin editor of any Atomic site.

Maybe we could add a new bundle that will be loaded by the block editor of Simple and Atomic sites (either iframed or not), and not loaded by Jetpack sites.

@gwwar
Copy link
Contributor

gwwar commented Jul 15, 2019

Would it be possible to simplify block-editor integration constraints @apeatling @gravityrail ?

We currently need to handle Simple in wp-admin/iframe, Atomic in wp-admin/iframe, Jetpack iframe/wp-admin (needs extra module checks?)

Edit: Overall it might make sense to group some of these common integrations on Jetpack to always adding them when we're editing at WordPress.com/block-editor vs wp-admin.

We gate in a similar way for the switch to classic option, with global vars

if ( ! wpcomGutenberg.switchToClassic.isVisible ) {

@simison
Copy link
Member Author

simison commented Oct 11, 2019

So might be that some features are meant to be shipped only on .com (like disabling Guten-NUX)

That does sound like an oversight; I'd expect the core NUX to not be on dotcom, but still presented on a Jetpack site. @mmtr am I remembering that correctly, I think you worked on that?..

Definitely an oversight. I can see a couple of integrations (disabling tips or unregistering experimental blocks) we shouldn't expose to Jetpack sites.

@gwwar et co do you know if these features were gated out from Jetpack plugin since I opened this issue, and is it on the roadmap?

@simison
Copy link
Member Author

simison commented Oct 11, 2019

We currently need to handle Simple in wp-admin/iframe, Atomic in wp-admin/iframe, Jetpack iframe/wp-admin (needs extra module checks?)

Since WordPress.com block editor iframe isn't used for self-hosted Jetpack sites (they get redirected to WP Admin instead), would it be safe to not load these files on those sites? They'd lose underline-text etc features but that's minor methinks.

@gwwar
Copy link
Contributor

gwwar commented Oct 11, 2019

@simison what features are you trying to sort out?

they get redirected to WP Admin instead

This is not true for all flows.

@mmtr
Copy link
Member

mmtr commented Oct 14, 2019

@simison what features are you trying to sort out?

I think there are only 2 features we don't want to expose in Jetpack WP Admin block editor: disabling guided tour and unregistering experimental blocks.

I still think we should split the existing common bundle into 2 different ones:

1st bundle (it can remains as common and loaded in any environment - iframed and not iframed):

  • Rich text (justify / underline).
  • Switch to classic.
  • Tracking.

2nd bundle (it can be called wpcom, to be loaded in Simple and Atomic sites only - iframed and not iframed):

  • Disable NUX tour.
  • Unregister experimental blocks.

@simison
Copy link
Member Author

simison commented Oct 14, 2019

disabling guided tour and unregistering experimental blocks.

Yep, those two I had in mind as well.

Would it make sense to have a separate bundle for jetpack self-hosted sites so that importing features to it would be very intentional and clear?

@gwwar
Copy link
Contributor

gwwar commented Oct 14, 2019

Would it make sense to have a separate bundle for jetpack self-hosted sites so that importing features to it would be very intentional and clear?

Did you have an example @simison ?

@mmtr or @simison, do folks mind updating the summary with the additional requirements/context, it'll save folks time when looking at the issue.

@mmtr
Copy link
Member

mmtr commented Oct 15, 2019

Would it make sense to have a separate bundle for jetpack self-hosted sites so that importing features to it would be very intentional and clear?

We don't have any specific feature for Jetpack sites only, so this would imply to duplicate imports and easy to miss.

I'd start with splitting the bundles (common, calypso, wpcom) and improving the docs to make clear where every bundle is loaded (that info is abstract in the current docs).

do folks mind updating the summary with the additional requirements/context

Done!

@simison
Copy link
Member Author

simison commented Oct 15, 2019

Would it make sense to have a separate bundle for jetpack self-hosted sites so that importing features to it would be very intentional and clear?

Did you have an example @simison ?

Just 3 entry points for each environment with duplicate imports for each feature.

We don't have any specific feature for Jetpack sites only, so this would imply to duplicate imports and easy to miss.

Valid point! I was thinking that would be an OK tradeoff in this case but if better documentation cuts it, that'd be great!

@gwwar
Copy link
Contributor

gwwar commented Oct 25, 2019

As an implementation note, we should probably keep the current common bundle filename for Jetpack sites, and create a new common file for Simple and Atomic sites. This is mainly because we keep one version of truth (not paired to releases) and we can control versioning for Simple and Atomic sites. Standalone Jetpack sites have a long tail for Jetpack plugin upgrades.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Goal] Gutenberg Working towards full integration with Gutenberg Jetpack [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants