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

Migrating experimental navigation and widgets screen to bulk REST API #24907

Closed
Tracked by #29102
adamziel opened this issue Aug 28, 2020 · 18 comments
Closed
Tracked by #29102

Migrating experimental navigation and widgets screen to bulk REST API #24907

adamziel opened this issue Aug 28, 2020 · 18 comments
Labels
[Feature] Widgets Screen The block-based screen that replaced widgets.php. [Package] Edit Widgets /packages/edit-widgets

Comments

@adamziel
Copy link
Contributor

Both navigation and widget management screens are interacting with the API using suboptimal methods. One of them reuses the customizer endpoint, the other sends multiple requests every time the user hits the "Save" button. Fortunately, the support for bulk API requests is coming as discussed in https://core.trac.wordpress.org/ticket/50244.

One problem there is that there will be a period of time when both screens are going to be available via Gutenberg plugin, but the bulk API support is only going to be available in latest WordPress trunk. This means that some users attempting to test the feature won't be able to use the full power of bulk API (including pre-validation), and will potentially experience partial errors on save.

There are a few ways to address it, the consensus at the moment seems to be improve the validation and catch most errors client-side before issuing any request. For the experimental feature it doesn't sound too bad. Even with 100-200 requests required to save the navigation, we could catch most errors client-side during the validation. As long as the user don’t close their browser during the save, it should work fine 90%+ of the time. Slowly, true, but consistently for the most part. The transition period will not be too long either, and once WordPress 5.6 gets released everyone will have access to the bulk API.

Does that make sense? Does anyone think there's a better way?

CC @draganescu @TimothyBJacobs @noisysocks @talldan @mtias

@adamziel adamziel added [Feature] Widgets Screen The block-based screen that replaced widgets.php. [Package] Edit Widgets /packages/edit-widgets [Feature] Navigation Screen labels Aug 28, 2020
@adamziel adamziel added this to Inbox in Block-based Widgets Editor via automation Aug 28, 2020
@noisysocks
Copy link
Member

Could we have Gutenberg polyfill the functionality added in Trac#50244 if it doesn't already exist?

@TimothyBJacobs
Copy link
Member

The plan is to polyfill as much of the functionality as possible, but it won't be able to support pre-validation because it requires extensive refactoring to WP_REST_Server::dispatch() that would be very difficult to do in Gutenberg.

@TimothyBJacobs TimothyBJacobs added this to Inbox in Navigation editor via automation Sep 5, 2020
@draganescu draganescu moved this from Inbox to Blocked in Block-based Widgets Editor Sep 6, 2020
@draganescu draganescu moved this from Inbox to Blocked in Navigation editor Sep 6, 2020
@draganescu
Copy link
Contributor

Blocked on #25096

@adamziel
Copy link
Contributor Author

adamziel commented Oct 9, 2020

Now that #25096 is merged this one may be picked up, although keep in mind it will be affected by #25958.

@adamziel adamziel moved this from Blocked to Overviews in Block-based Widgets Editor Oct 9, 2020
@adamziel adamziel moved this from Overviews to Inbox in Block-based Widgets Editor Oct 9, 2020
@adamziel
Copy link
Contributor Author

adamziel commented Oct 9, 2020

The more I think about it the more I'm convinced that the widgets editor should address this in the same (or related) PR as the migration to #25958.

@adamziel
Copy link
Contributor Author

This is now ready for dev since #25958 and #26042 got merged. Two related issues/PRs that could potentially address it:

@adamziel adamziel moved this from Inbox to Design in Block-based Widgets Editor Oct 14, 2020
@adamziel adamziel moved this from Design to Needs dev in Block-based Widgets Editor Oct 14, 2020
@noisysocks noisysocks moved this from Needs dev to Issues in progress in Block-based Widgets Editor Oct 19, 2020
@noisysocks noisysocks moved this from Blocked to Issues in progress in Navigation editor Oct 19, 2020
@adamziel
Copy link
Contributor Author

This PR could affect batch processing: #26389

@noisysocks
Copy link
Member

Just flagging a thought I had: If we implement previewing on this screen (and I think we should), we'll probably want to use a REST API for creating and publishing changesets instead of the batch processing API.

https://core.trac.wordpress.org/ticket/38900

@noisysocks noisysocks moved this from Issues in progress to Needs dev in Block-based Widgets Editor Jan 5, 2021
@noisysocks
Copy link
Member

Unassigning @adamziel as I know he's moved onto bigger and better things 😛

@noisysocks
Copy link
Member

noisysocks commented Jan 28, 2021

I'd consider this ✅ done for the widgets screen. #28210 rewrote the batch processor into something which I think can eventually serve as the final API.

I'll leave the issue open though since I think the navigation screen needs to be updated cc. @talldan.

@noisysocks noisysocks moved this from Needs dev to Done in Block-based Widgets Editor Jan 28, 2021
@draganescu draganescu moved this from Issues in progress to Needs dev in Navigation editor Feb 18, 2021
@talldan
Copy link
Contributor

talldan commented Apr 27, 2021

As mentioned above, batch saving was implemented as part of the widgets work (#28210).

For menus, I think the reorder endpoint is currently needed to allow saving via the REST API - #25093

There’s some more background about why this is required in #21964.

@spacedmonkey
Copy link
Member

Re-sharing here for visibility.

  1. The menu items endpoint, already supports batching and has done for a while.
  2. Current thinking is we should just use batching functionally and not bother with the reordering endpoint. See REST API: Introduce reorder menu endpoint #25093 (comment)

@spacedmonkey
Copy link
Member

@adamziel @TimothyBJacobs Can we close this ticket now that #34541 is merged?

@TimothyBJacobs
Copy link
Member

I believe so, I'd wait on Adam to confirm though.

@adamziel
Copy link
Contributor Author

adamziel commented Sep 22, 2021

Thanks for staying on top of that! Yup, we're good here. There are a few follow-up items, but I believe we could track them separately. I'll create dedicated issues today.

Navigation editor automation moved this from Needs dev to Done Sep 22, 2021
@adamziel
Copy link
Contributor Author

adamziel commented Sep 22, 2021

I think these are all the follow-up items:

#35048
#35047
#35046
#35045
#35037
#34999

Edit: Any idea how to make GitHub expand ticket numbers into descriptions?

@Mamaduka
Copy link
Member

Thanks, @adamziel

Maybe we should create separate tracking ticket for data-related issues.

P.S. I think GitHub only expands ticket numbers when used with task list - [ ]

@adamziel
Copy link
Contributor Author

@Mamaduka

Maybe we should create separate tracking ticket for data-related issues.

Which ones specifically?

P.S. I think GitHub only expands ticket numbers when used with task list - [ ]

ah that's good to know, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Widgets Screen The block-based screen that replaced widgets.php. [Package] Edit Widgets /packages/edit-widgets
Projects
No open projects
Development

No branches or pull requests

7 participants