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

Metaboxes: Prefetch Metaboxes and don't reload them on save #3616

Merged
merged 1 commit into from
Dec 6, 2017

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Nov 22, 2017

The idea is that the metaboxes needs to be loaded with Gutenberg on initial load to initialize properly and their HTML don't need to be updated on Save.

So this does:

  • Prefetch the metaboxes HTML when loading Gutenberg
  • When showing "the metabox area", the dom node are moved from the "prefetch" zone to the the "metabox area" and when the "metabox area" is unmounted (for example, closing the sidebar) the "prefetched metaboxes" are moved back to the "prefetch" zone.
  • When saving a post with metaboxes, an AJAX request is triggered to save the metaboxes but the returned HTML is ignored.

closes #3523 #3277

@codecov
Copy link

codecov bot commented Nov 22, 2017

Codecov Report

Merging #3616 into master will decrease coverage by 0.32%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3616      +/-   ##
==========================================
- Coverage   37.74%   37.41%   -0.33%     
==========================================
  Files         279      277       -2     
  Lines        6743     6676      -67     
  Branches     1227     1209      -18     
==========================================
- Hits         2545     2498      -47     
+ Misses       3536     3525      -11     
+ Partials      662      653       -9
Impacted Files Coverage Δ
editor/effects.js 63.15% <ø> (+5.58%) ⬆️
...tor/components/meta-boxes/meta-boxes-area/index.js 0% <0%> (ø) ⬆️
components/panel/row.js 0% <0%> (-100%) ⬇️
editor/components/warning/index.js 0% <0%> (-100%) ⬇️
components/panel/color.js 0% <0%> (-100%) ⬇️
blocks/autocompleters/index.js 0% <0%> (-42.11%) ⬇️
editor/utils/mobile/index.js 57.14% <0%> (-17.86%) ⬇️
editor/components/post-publish-button/label.js 83.33% <0%> (-4.17%) ⬇️
blocks/library/quote/index.js 18.42% <0%> (-3.46%) ⬇️
blocks/editable/format-toolbar/index.js 6.38% <0%> (-1.96%) ⬇️
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e60bd7...3639678. Read the comment docs.

@youknowriad
Copy link
Contributor Author

I like how this removes a lot of code :)

@BE-Webdesign
Copy link
Contributor

Yeah, going to check this out more in depth, but now that we are on post.php this is more the approach we should head. We don't need to fake things anymore since we are just on post.php. Great stuff so far!

@youknowriad
Copy link
Contributor Author

Do you all agree on moving forward with this direction?

* @param array $wp_meta_boxes Global meta box state.
*/
$wp_meta_boxes = apply_filters( 'filter_gutenberg_meta_boxes', $wp_meta_boxes );

Copy link
Member

Choose a reason for hiding this comment

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

Now that we're loading it inline, we should call gutenberg_intercept_meta_box_render() here - it should be done immediately before the do_meta_boxes() calls.

I'm inclined to not attach it to filter_gutenberg_meta_boxes, to prevent anything from being added to $wp_meta_boxes after it runs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought the gutenberg_intercept_meta_box_render was meant to display a warning in the classic editor while the_gutenberg_metaboxes renders the metaboxes in Gutenberg.

Am I missing something?

Anyway, feel free to push any change to this branch

Copy link
Member

Choose a reason for hiding this comment

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

Oh, wait. Yeah, I totally wasn't reading it right. 🙂

@youknowriad youknowriad force-pushed the try/prefetch-dont-reload-metaboxees branch from aafe321 to 07187b6 Compare November 27, 2017 11:07
@mtias mtias added the [Feature] Meta Boxes A draggable box shown on the post editing screen label Nov 27, 2017
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

To be clear, is the idea that we're abandoning any attempt to keep the meta boxes up to date since they don't always work well with auto-save or otherwise long-lived page sessions?

<input type="hidden" id="referredby" name="referredby" value="<?php echo $referer ? esc_url( $referer ) : ''; ?>" />
<!-- These fields are not part of the standard post form. Used to redirect back to this page on save. -->
<input type="hidden" name="gutenberg_meta_boxes" value="gutenberg_meta_boxes" />
<input type="hidden" name="gutenberg_meta_box_location" value="<?php echo esc_attr( $location ); ?>" />
Copy link
Member

Choose a reason for hiding this comment

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

Notice: Undefined variable: location in /srv/www/editor/htdocs/wp-content/plugins/gutenberg/lib/meta-box-partial-page.php on line 390

@youknowriad
Copy link
Contributor Author

To be clear, is the idea that we're abandoning any attempt to keep the meta boxes up to date

I'd not phrase it like this, There are two things:

  • Prefetching metaboxes from the server when loading the gutenberg page. This makes sure all metaboxes initialization scripts run just like before

  • Do not refresh the HTML for the metaboxes on save. I personally haven't encoutered any metaboxes where this cause issues. Most metaboxes refresh with JavaScript or are forms that update with user input. And updating the HTML on save was broken anyway because there's no way to ensure the initialization scripts run again on success.

@aduth
Copy link
Member

aduth commented Nov 27, 2017

And updating the HTML on save was broken anyway because there's no way to ensure the initialization scripts run again on success.

I guess this was easier with iframes, since they were standalone documents, but is there no way to mimic the document readying events for plugin metaboxes?

@youknowriad
Copy link
Contributor Author

but is there no way to mimic the document readying events for plugin metaboxes?

Nothing that I'm aware of. Even regular onload events were not being called on "initial rendering".

@youknowriad youknowriad force-pushed the try/prefetch-dont-reload-metaboxees branch 3 times, most recently from 8014329 to 10acc3e Compare November 28, 2017 08:25
@youknowriad youknowriad force-pushed the try/prefetch-dont-reload-metaboxees branch from 10acc3e to 3639678 Compare December 6, 2017 08:16
@youknowriad
Copy link
Contributor Author

Merging. Most concerns addressed and the code is way cleaner with this

@youknowriad youknowriad merged commit 40f89a8 into master Dec 6, 2017
@youknowriad youknowriad deleted the try/prefetch-dont-reload-metaboxees branch December 6, 2017 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Meta Boxes A draggable box shown on the post editing screen
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Version 1.7 metaboxes refresh and break functionality
5 participants