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

Try Legacy widget block #13511

Open
wants to merge 9 commits into
base: master
from

Conversation

@jorgefilipecosta
Copy link
Member

jorgefilipecosta commented Jan 25, 2019

Description

Implements: #4770

This PR is a proof of concept of a legacy widget block. A block that allows existing WordPress widgets to be added as Gutenberg blocks.

The design is similar to the one proposed by @melchoyce in #4770 (comment) (option 1). Although it seems option two was preferred, it would require to expose each widget as a block similar to what embeds do, and it would increase the technical complexity and make testing/debugging harder, so I preferred to use option 1 for now. I will gladly iterate on the UX after this proof of concept gets more stable.

Some technical details

The available widgets are preloaded to Gutenberg similar to what happens with page templates.

REST-API user story

I want to able to pass a widget identifier to an endpoint, pass the existing widget attributes and the changes the user is making if any, and receive from the rest API, the sanitized new widget attributes ready to save and an HTML form that allows the user to edit the widget in the new state.

REST-API endpoint

A very simple REST-API endpoint was implemented. The endpoint receives the previous instance of a widget (previous attributes) the changed instance of a widget (changed attributes if any) and returns the new instance of the widget and the HTML form that allows editing this widget.
There are two ajax-admin endpoints save-widget and update-widget. It looks like each one has specificities that make using it here complex. The ajax admin code would probably require some changes to be used here. Our use case is straightforward from the backend perspective as the widget does not need to be saved anywhere, and the widget is not associated with any widget area. The most straightforward approach seemed to be using a very simple endpoint. If I missed something and adapting existing endpoints is simpler feel free to comment, and I will have a look.

Block Architecture

The edit component of the block handles the start placeholder that allows selecting a widget, and the tab mechanism that allows switching between edit and preview.
The preview is done using the ServerSideRender component.
The edit is done using two components:
WidgetEditHandler: Is responsible for server communication using the endpoint we created, and for keeping the required local state for the edition. Renders an update button, when pressed we retrieve from the dom the changed fields (using a method provided by WidgetEditDomManager) issues a request to the server and updates the legacy widget instance attribute with the server answer.
WidgetEditDomManager: Component responsible for rendering the starting dom for a widget. After the first render React never rerenders the component again, the content rendered by this component is then managed by the scripts the widgets may implement. When a new instance of the form HTML is received we manually update the dom changing .widget-content (like the customizer and the widget screen) do. This component provides a method that returns an object with the widget changed attributes. When this component is mounted it triggers a widget-added event when a new update happens and the dom is changed by the component widget-update jQuery event is triggered.

On front end widget are rendered with a simple call to the_widget.

Screenshots

jan-25-2019 19-37-44
jan-25-2019 18-58-49

Known problems

  • The block is not aware of any change inside the widget until the update button is pressed. This replicates the save button on the widgets screen. But it is annoying if we change something on the widget and go to the widget preview right away our changes are not reflected in the preview. Having an explicit update, makes testing and debugging easier, we may than explore other approaches e.g.: also save when preview happens, save on blur events, etc.

  • The text widget that contains TinyMCE crashes and fails to init. It calls wp.editor.initialize to reference TinyMCE and on Gutenberg, wp.editor is our editor module. This problem may have happened with meta boxes if it was solved probably the same approach may be applied.

  • The widget design may be affected by CSS that exists in Gutenberg, so the design of the widgets does not look the same. Ideally, Gutenberg CSS would not affect the widgets but as they are on the same page that's not the case.

  • Some third-party widgets using don't initialize correctly. That happens because the dom of the editor is not equal to the dom of the customizer and/or the widget screen. Some JS widgets use click events on the widgets screen to initialize while on Gutenberg these events don't happen, some check if they are on the customizer page (body contains customizer classes) before handling widget-updated events. Normally adapting a widget that does not initialize correctly is a matter of changing a very simple condition on the plugin.

@jorgefilipecosta jorgefilipecosta force-pushed the try/legacy-widget-block branch from 4af57d5 to 35e0180 Jan 25, 2019

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jan 29, 2019

Awesome work here. What's the plan to move forward here.

  • Should we try to split it into smaller steps? The endpoint could be built separately for instance.
  • Also this would be a good use-case for the feature flags #13324

And let's get some technical feedback here.

@youknowriad youknowriad requested a review from WordPress/gutenberg-core Jan 29, 2019

@afercia afercia referenced this pull request Jan 30, 2019

Open

Iterations on "Latest Posts" Block #1594

4 of 9 tasks complete

@melchoyce melchoyce added this to In Progress in Porting widgets to blocks Jan 30, 2019

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

jorgefilipecosta commented Jan 30, 2019

What's the plan to move forward here.

Should we try to split it into smaller steps? The endpoint could be built separately for instance.
Also this would be a good use-case for the feature flags #13324
And let's get some technical feedback here.

I think it should be divided into 3 parts:

  • PR's that add enhancements but to allow this feature but are not directly related to it. Already in progress.
  • The REST endpoint
  • The legacy widget block + loading of widget assets + preloading available widgets.

The third part although smaller than this PR is still huge, but I don't think there is a possible usable logical division for this part.

I agree this provides a good use case for feature flags. Besides conditional JS code, we may also need some conditional PHP code. I'm not sure if feature flags will/should touch PHP logic. Probably for PHP code we can keep it in the plugin without conditions and don't merge it to the core. It will be an additional source of friction if later we need to move some PHP code to core as we will need to carefully select the parts of the code that will be moved.

@mapk

This comment has been minimized.

Copy link
Contributor

mapk commented Jan 31, 2019

@jorgefilipecosta I tried it out and am missing all the in-block widget settings. Am I missing something?

legacy-widget

@jorgefilipecosta jorgefilipecosta force-pushed the try/legacy-widget-block branch from 35e0180 to 790b2d5 Jan 31, 2019

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

jorgefilipecosta commented Jan 31, 2019

@jorgefilipecosta I tried it out and am missing all the in-block widget settings. Am I missing something?

Hi @mapk, the widget settings at least for the core blocks should just work and appear without problems. I tested in chrome and safari and they are working correctly in my case.
I rebased this PR let me know if in your tests things are still not working as expected. Also if possible let me know what WordPress version you are testing this on, and if you have any other plugins installed.

@mapk

This comment has been minimized.

Copy link
Contributor

mapk commented Jan 31, 2019

@jorgefilipecosta I've got the docker install of WordPress 5.0.3 running as outlined in the Contributions doc. There are a lot of Gutenberg Test ... plugins, but I haven't installed any others. Still experiencing the problem.

@jorgefilipecosta jorgefilipecosta force-pushed the try/legacy-widget-block branch 2 times, most recently from 0323d29 to 1fc4c1f Feb 1, 2019

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

jorgefilipecosta commented Feb 1, 2019

Hi @mapk, It turns out that my WordPress local install was modified with a display: none commented that I need to overwrite here. My last push adds a display: block; that should fix the problem. I'm sorry for the trouble caused. Let me know if it is possible to see the forms now.

@mapk

This comment has been minimized.

Copy link
Contributor

mapk commented Feb 4, 2019

I'll leave a few high level design comments here for ya!

1. There's no 'update' feedback for the user.
It would be great to have the 'update' button indicate a successful update to the user (ie. green checkmark, notification,etc.). Whatever is the Gutenberg standard.

no-update-feedback

@mapk

This comment has been minimized.

Copy link
Contributor

mapk commented Feb 4, 2019

2. The two "edit" buttons are a bit awkward.
I see an "Edit" icon which allows me to completely change the widget I want to use, and an "Edit" text button that let's me adjust the settings to that widget. Maybe the first icon can be something different indicating a change of widget? Some like the block transform icon? I'm not sure yet.

edit-edit

3. Select to change widget loses settings.
If I made a series of settings changes on a widget, and switched that widget, but wanted to keep the same widget I had, all the settings are gone. I'd like a way to retain my settings if possible.

@melchoyce

This comment has been minimized.

Copy link
Contributor

melchoyce commented Feb 4, 2019

I don't know that we lost any settings when converting core widgets to blocks — what if we didn't include them here, and only used it to display third-party widgets?

@gziolo gziolo self-requested a review Feb 5, 2019

@jorgefilipecosta jorgefilipecosta requested a review from westonruter Feb 5, 2019

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Feb 6, 2019

For the REST API endpoint, you can cross-reference with some prior art in https://github.com/xwp/wp-js-widgets/blob/develop/php/class-js-widgets-rest-controller.php

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Feb 6, 2019

Is there any need for the Update button? The Customizer normally does not include it, for example. It just listens to input events on the fields, and then updates the control's corresponding setting accordingly:

https://github.com/WordPress/wordpress-develop/blob/5f38ce3d9816efad8cfb0cd3f4af89591406a40d/src/js/_enqueues/wp/customize/widgets.js#L932-L940

https://github.com/WordPress/wordpress-develop/blob/5f38ce3d9816efad8cfb0cd3f4af89591406a40d/src/js/_enqueues/wp/customize/widgets.js#L1143-L1331

@gziolo gziolo added this to the 5.2 (Gutenberg) milestone Feb 7, 2019

@jorgefilipecosta jorgefilipecosta force-pushed the try/legacy-widget-block branch from bad0884 to bcf144c Feb 8, 2019

@jorgefilipecosta jorgefilipecosta requested a review from aduth as a code owner Feb 8, 2019

@jorgefilipecosta jorgefilipecosta force-pushed the try/legacy-widget-block branch 3 times, most recently from fbf4e43 to 976207a Feb 14, 2019

@earnjam
Copy link
Contributor

earnjam left a comment

Not sure if this is an issue with ServerSideRender or here without doing some further investigation, but when the widget does not return any rendered output, it just shows the spinning loading placeholder and never displays anything in the preview. Spinner implies it's still waiting on something, so I think we'll want some kind of message there. I tested it with the Tag Cloud widget and didn't have any tags assigned to posts.

Seems like some legacy widgets could have conditional output that would sometimes return nothing.

legacy-widget-null-response

@jorgefilipecosta jorgefilipecosta force-pushed the try/legacy-widget-block branch from 976207a to 2e5e4b8 Feb 18, 2019

@jorgefilipecosta jorgefilipecosta force-pushed the try/legacy-widget-block branch from 2e5e4b8 to 1cdc34b Feb 18, 2019

@jorgefilipecosta jorgefilipecosta requested review from nerrad and ntwb as code owners Feb 18, 2019

@jorgefilipecosta jorgefilipecosta force-pushed the try/legacy-widget-block branch from b66ecc3 to 98cbbf1 Feb 18, 2019

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

jorgefilipecosta commented Feb 18, 2019

I updated the PR:

  • I added permissions check.
  • I made the block experimental to warn users it is not stable yet.
  • I added feature flags.

Users that don't have permissions to access the widget screen will not be able to set a widget on the legacy widget block. If they are editing a post with a legacy widget already inserted in posts they have permission to edit they can see the preview of the widget but not change the widget and are not able to change its settings.

We don't have PHP feature flags so I added comment blocks to make porting code to core in a release that doesn't include this phase 2 item easier.
cc: @youknowriad

Hi @earnjam,

Not sure if this is an issue with ServerSideRender or here without doing some further investigation, but when the widget does not return any rendered output, it just shows the spinning loading placeholder and never displays anything in the preview.

This problem is caused by the ServerSideRender and we have an issue for it #13870. Given the already big size of this PR it is better to address this issue in a different PR.

@earnjam

This comment has been minimized.

Copy link
Contributor

earnjam commented Feb 18, 2019

Thanks @jorgefilipecosta! I didn't know about that issue. I agree it should be addressed outside of this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment