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

Reusable Blocks: Support importing and exporting reusable blocks #9788

Merged
merged 8 commits into from Sep 14, 2018

Conversation

Projects
None yet
6 participants
@youknowriad
Contributor

youknowriad commented Sep 11, 2018

This PR adds Export/Import capabilities to the page listing the available reusable blocks.

Remaining

  • Polish the design (especially the import form)
  • E2e tests

@youknowriad youknowriad self-assigned this Sep 11, 2018

@youknowriad youknowriad requested review from mtias, jasmussen and WordPress/gutenberg-core Sep 11, 2018

reader.onload = function() {
resolve( reader.result );
};
reader.readAsText( file );

This comment has been minimized.

@youknowriad

youknowriad Sep 11, 2018

Contributor

I have to check browser support for this one to see if we need a polyfill.

@youknowriad

youknowriad Sep 11, 2018

Contributor

I have to check browser support for this one to see if we need a polyfill.

This comment has been minimized.

@aduth

aduth Sep 14, 2018

Member

I have to check browser support for this one to see if we need a polyfill.

Did you check? As best I can tell from browser support resources, it should be supported.

https://developer.mozilla.org/en-US/docs/Web/API/FileReader/readAsText#Browser_compatibility

@aduth

aduth Sep 14, 2018

Member

I have to check browser support for this one to see if we need a polyfill.

Did you check? As best I can tell from browser support resources, it should be supported.

https://developer.mozilla.org/en-US/docs/Web/API/FileReader/readAsText#Browser_compatibility

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 11, 2018

Contributor

This is amazing. If you were more on fire, you'd cause nuclear fusion and solve the world energy crisis. Totally love it. Thanks so much for working on this.

A few screenshots.

  1. This is where management happens:

screen shot 2018-09-11 at 14 25 28

  1. This is the management page:

screen shot 2018-09-11 at 14 25 36

  1. When you press "Export", a JSON file is immediately downloaded. Here's what a JSON file could look like:
{
  "title": "Hello World",
  "content": "<!-- wp:paragraph -->\n<p>This is really dang cool.</p>\n<!-- /wp:paragraph -->"
}
  1. If you presse Import, this is what you see:

screen shot 2018-09-11 at 14 25 45

  1. I got a small bug after importing:

screen shot 2018-09-11 at 14 25 58

But overall, this is SO SO SO VERY SOLID. It works remarkably well, and I really dig how you've gone for the simplest possible solution here. Along with the multi-selection reusable blocks PR, this is going to be magical.


It's also so magical that my excitement is getting the better of me. So please categorize my feedback into two categories, soon and future.

Things we should do soon, if not in this PR:

2, 3, 4 are all fine.

5 obviously needs to be fixed.

1 is the weakest part, simply because it's so hard to find out where to manage your reusable blocks. I think it's probably fine to have the cog in the reusable blocks panel, but the feature is becoming so cool I feel it needs better placement. The first place I would look for this is in the More menu. We could have an item there that simply says "Manage reusable blocks".

Or, we could have multiple items there — Manage Reusable Blocks, Import Blocks, Export Blocks. Some of these items also depend on where we go with #9732. For example I think it would be neat to default to importing blocks that are immediately editable, i.e. they are not reusable. And then repositioning "reusable" to be an addition on top of that. They could even be renamed Templates (immediately editable blocks) and Global Templates (new name for reusable blocks).

In such a situation perhaps the page could be "Manage Block Templates".

Things we should think about and maybe do afterwards, or in the future:

  • Wouldn't it be cool if you could drag and drop a .json file directly on to the editing canvas, resulting in a prompt asking you if you wanted to [Import Block Template] or [Import and insert Block Template]?
  • I personally have a great desire to copy and paste these Template json files intead of having to manage local saved .json files. I imagine people sharing these snippets via pastebin on Twitter, or in lots of different places, and I imagine the increased sharability of a string of text than a file is hugely valuable. As such, it would be nice if pressing "Export" opened a modal prompt asking you if you wanted to download the .json file, or copy it to your clipboard. Similarly, pressing Import could open a modal asking if you wanted to paste it in, or upload a .json file.
  • Since we're in "crazy dreaming" territory, how cool would it be if you could paste this JSON directly into the editor in an empty paragraph, and our paste filters detected and imported it on the fly?

Okay I'm going to splash some cold water on my face.

Immediate next step to me seems like it should be to copy the "Manage Reusable Blocks" link to the More menu in the to right corner. With that in place (and 5 fixed), I think this could actually be good to go.

Contributor

jasmussen commented Sep 11, 2018

This is amazing. If you were more on fire, you'd cause nuclear fusion and solve the world energy crisis. Totally love it. Thanks so much for working on this.

A few screenshots.

  1. This is where management happens:

screen shot 2018-09-11 at 14 25 28

  1. This is the management page:

screen shot 2018-09-11 at 14 25 36

  1. When you press "Export", a JSON file is immediately downloaded. Here's what a JSON file could look like:
{
  "title": "Hello World",
  "content": "<!-- wp:paragraph -->\n<p>This is really dang cool.</p>\n<!-- /wp:paragraph -->"
}
  1. If you presse Import, this is what you see:

screen shot 2018-09-11 at 14 25 45

  1. I got a small bug after importing:

screen shot 2018-09-11 at 14 25 58

But overall, this is SO SO SO VERY SOLID. It works remarkably well, and I really dig how you've gone for the simplest possible solution here. Along with the multi-selection reusable blocks PR, this is going to be magical.


It's also so magical that my excitement is getting the better of me. So please categorize my feedback into two categories, soon and future.

Things we should do soon, if not in this PR:

2, 3, 4 are all fine.

5 obviously needs to be fixed.

1 is the weakest part, simply because it's so hard to find out where to manage your reusable blocks. I think it's probably fine to have the cog in the reusable blocks panel, but the feature is becoming so cool I feel it needs better placement. The first place I would look for this is in the More menu. We could have an item there that simply says "Manage reusable blocks".

Or, we could have multiple items there — Manage Reusable Blocks, Import Blocks, Export Blocks. Some of these items also depend on where we go with #9732. For example I think it would be neat to default to importing blocks that are immediately editable, i.e. they are not reusable. And then repositioning "reusable" to be an addition on top of that. They could even be renamed Templates (immediately editable blocks) and Global Templates (new name for reusable blocks).

In such a situation perhaps the page could be "Manage Block Templates".

Things we should think about and maybe do afterwards, or in the future:

  • Wouldn't it be cool if you could drag and drop a .json file directly on to the editing canvas, resulting in a prompt asking you if you wanted to [Import Block Template] or [Import and insert Block Template]?
  • I personally have a great desire to copy and paste these Template json files intead of having to manage local saved .json files. I imagine people sharing these snippets via pastebin on Twitter, or in lots of different places, and I imagine the increased sharability of a string of text than a file is hugely valuable. As such, it would be nice if pressing "Export" opened a modal prompt asking you if you wanted to download the .json file, or copy it to your clipboard. Similarly, pressing Import could open a modal asking if you wanted to paste it in, or upload a .json file.
  • Since we're in "crazy dreaming" territory, how cool would it be if you could paste this JSON directly into the editor in an empty paragraph, and our paste filters detected and imported it on the fly?

Okay I'm going to splash some cold water on my face.

Immediate next step to me seems like it should be to copy the "Manage Reusable Blocks" link to the More menu in the to right corner. With that in place (and 5 fixed), I think this could actually be good to go.

@ZebulanStanphill

This comment has been minimized.

Show comment
Hide comment
@ZebulanStanphill

ZebulanStanphill Sep 11, 2018

Contributor

This is a great addition!

@jasmussen

I think it's probably fine to have the cog in the reusable blocks panel, but the feature is becoming so cool I feel it needs better placement. The first place I would look for this is in the More menu. We could have an item there that simply says "Manage reusable blocks".

I agree that making this feature more visible is a good idea.

They could even be renamed Templates (immediately editable blocks) and Global Templates (new name for reusable blocks).

Speaking of using reusable blocks as templates: #8403. I recommend checking out how Beaver Builder, Divi, and Oxygen implement this kind of thing for some inspiration.

  • Wouldn't it be cool if you could drag and drop a .json file directly on to the editing canvas, resulting in a prompt asking you if you wanted to [Import Block Template] or [Import and insert Block Template]?
  • I personally have a great desire to copy and paste these Template json files intead of having to manage local saved .json files. I imagine people sharing these snippets via pastebin on Twitter, or in lots of different places, and I imagine the increased sharability of a string of text than a file is hugely valuable. As such, it would be nice if pressing "Export" opened a modal prompt asking you if you wanted to download the .json file, or copy it to your clipboard. Similarly, pressing Import could open a modal asking if you wanted to paste it in, or upload a .json file.
  • Since we're in "crazy dreaming" territory, how cool would it be if you could paste this JSON directly into the editor in an empty paragraph, and our paste filters detected and imported it on the fly?

Yes to all of this!

Immediate next step to me seems like it should be to copy the "Manage Reusable Blocks" link to the More menu in the to right corner. With that in place (and 5 fixed), I think this could actually be good to go.

Sounds good to me!

Contributor

ZebulanStanphill commented Sep 11, 2018

This is a great addition!

@jasmussen

I think it's probably fine to have the cog in the reusable blocks panel, but the feature is becoming so cool I feel it needs better placement. The first place I would look for this is in the More menu. We could have an item there that simply says "Manage reusable blocks".

I agree that making this feature more visible is a good idea.

They could even be renamed Templates (immediately editable blocks) and Global Templates (new name for reusable blocks).

Speaking of using reusable blocks as templates: #8403. I recommend checking out how Beaver Builder, Divi, and Oxygen implement this kind of thing for some inspiration.

  • Wouldn't it be cool if you could drag and drop a .json file directly on to the editing canvas, resulting in a prompt asking you if you wanted to [Import Block Template] or [Import and insert Block Template]?
  • I personally have a great desire to copy and paste these Template json files intead of having to manage local saved .json files. I imagine people sharing these snippets via pastebin on Twitter, or in lots of different places, and I imagine the increased sharability of a string of text than a file is hugely valuable. As such, it would be nice if pressing "Export" opened a modal prompt asking you if you wanted to download the .json file, or copy it to your clipboard. Similarly, pressing Import could open a modal asking if you wanted to paste it in, or upload a .json file.
  • Since we're in "crazy dreaming" territory, how cool would it be if you could paste this JSON directly into the editor in an empty paragraph, and our paste filters detected and imported it on the fly?

Yes to all of this!

Immediate next step to me seems like it should be to copy the "Manage Reusable Blocks" link to the More menu in the to right corner. With that in place (and 5 fixed), I think this could actually be good to go.

Sounds good to me!

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 13, 2018

Contributor
  • I added the more menu item to "Manage reusable blocks"
  • I fixed the bug on import success. A compromise I made though is to avoid refreshing the page on a successful import. The idea is that later on, we probably would avoid the PHP generated page entirely so instead of battling and trying to find a way to properly refresh and show the notice, we should the success notice and at some point when the refactoring of this page is done to be full JS rendered, it would be easy to just refresh the listing.
  • I added an explicit __file: 'wp_block' tag to the JSON exported file to avoid ambiguity about the file's content.
Contributor

youknowriad commented Sep 13, 2018

  • I added the more menu item to "Manage reusable blocks"
  • I fixed the bug on import success. A compromise I made though is to avoid refreshing the page on a successful import. The idea is that later on, we probably would avoid the PHP generated page entirely so instead of battling and trying to find a way to properly refresh and show the notice, we should the success notice and at some point when the refactoring of this page is done to be full JS rendered, it would be easy to just refresh the listing.
  • I added an explicit __file: 'wp_block' tag to the JSON exported file to avoid ambiguity about the file's content.

@youknowriad youknowriad added this to the 3.9 milestone Sep 13, 2018

@aduth aduth self-requested a review Sep 13, 2018

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 13, 2018

Member

What are testing instructions here?

Trying to infer from code, when navigating to /wp-admin/edit.php?post_type=wp_block, clicking the Import from JSON button causes the button to disappear, with a few errors in console:

Uncaught TypeError: Cannot set property isMounted of #<Component> which has only a getter
    at new ImportForm (index.js:23)
    at constructClassInstance (react-dom.24169eaf.js:12025)
    at updateClassComponent (react-dom.24169eaf.js:13722)
    at beginWork (react-dom.24169eaf.js:14402)
    at performUnitOfWork (react-dom.24169eaf.js:16441)
    at workLoop (react-dom.24169eaf.js:16480)
    at HTMLUnknownElement.callCallback (react-dom.24169eaf.js:140)
    at Object.invokeGuardedCallbackDev (react-dom.24169eaf.js:178)
    at invokeGuardedCallback (react-dom.24169eaf.js:227)
    at replayUnitOfWork (react-dom.24169eaf.js:15888)
ImportForm @ index.js:23
constructClassInstance @ react-dom.24169eaf.js:12025
updateClassComponent @ react-dom.24169eaf.js:13722
beginWork @ react-dom.24169eaf.js:14402
performUnitOfWork @ react-dom.24169eaf.js:16441
workLoop @ react-dom.24169eaf.js:16480
callCallback @ react-dom.24169eaf.js:140
invokeGuardedCallbackDev @ react-dom.24169eaf.js:178
invokeGuardedCallback @ react-dom.24169eaf.js:227
replayUnitOfWork @ react-dom.24169eaf.js:15888
renderRoot @ react-dom.24169eaf.js:16540
performWorkOnRoot @ react-dom.24169eaf.js:17138
performWork @ react-dom.24169eaf.js:17060
performSyncWork @ react-dom.24169eaf.js:17032
interactiveUpdates$1 @ react-dom.24169eaf.js:17297
interactiveUpdates @ react-dom.24169eaf.js:2326
dispatchInteractiveEvent @ react-dom.24169eaf.js:4882

react-dom.24169eaf.js:14804 The above error occurred in the <ImportForm> component:
    in ImportForm (created by WithInstanceId(ImportForm))
    in WithInstanceId(ImportForm) (created by Dropdown)
    in div (created by Popover)
    in div (created by Popover)
    in PopoverDetectOutside (created by WrappedPopoverDetectOutside)
    in WrappedPopoverDetectOutside (created by Popover)
    in Unknown (created by WithFocusReturn(Component))
    in div (created by WithFocusReturn(Component))
    in WithFocusReturn(Component) (created by WithConstrainedTabbing(WithFocusReturn(Component)))
    in div (created by WithConstrainedTabbing(WithFocusReturn(Component)))
    in WithConstrainedTabbing(WithFocusReturn(Component)) (created by Popover)
    in span (created by Popover)
    in Popover (created by Dropdown)
    in div (created by Dropdown)
    in Dropdown (created by ImportDropdown)
    in ImportDropdown

Consider adding an error boundary to your tree to customize error handling behavior.
Visit https://fb.me/react-error-boundaries to learn more about error boundaries.
logCapturedError @ react-dom.24169eaf.js:14804
logError @ react-dom.24169eaf.js:14843
update.callback @ react-dom.24169eaf.js:15496
callCallback @ react-dom.24169eaf.js:11456
commitUpdateQueue @ react-dom.24169eaf.js:11500
commitLifeCycles @ react-dom.24169eaf.js:14974
commitAllLifeCycles @ react-dom.24169eaf.js:16040
callCallback @ react-dom.24169eaf.js:140
invokeGuardedCallbackDev @ react-dom.24169eaf.js:178
invokeGuardedCallback @ react-dom.24169eaf.js:227
commitRoot @ react-dom.24169eaf.js:16181
completeRoot @ react-dom.24169eaf.js:17196
performWorkOnRoot @ react-dom.24169eaf.js:17141
performWork @ react-dom.24169eaf.js:17060
performSyncWork @ react-dom.24169eaf.js:17032
interactiveUpdates$1 @ react-dom.24169eaf.js:17297
interactiveUpdates @ react-dom.24169eaf.js:2326
dispatchInteractiveEvent @ react-dom.24169eaf.js:4882

react-dom.24169eaf.js:17121 Uncaught TypeError: Cannot set property isMounted of #<Component> which has only a getter
    at new ImportForm (index.js:23)
    at constructClassInstance (react-dom.24169eaf.js:12025)
    at updateClassComponent (react-dom.24169eaf.js:13722)
    at beginWork (react-dom.24169eaf.js:14402)
    at performUnitOfWork (react-dom.24169eaf.js:16441)
    at workLoop (react-dom.24169eaf.js:16480)
    at renderRoot (react-dom.24169eaf.js:16520)
    at performWorkOnRoot (react-dom.24169eaf.js:17138)
    at performWork (react-dom.24169eaf.js:17060)
    at performSyncWork (react-dom.24169eaf.js:17032)
Member

aduth commented Sep 13, 2018

What are testing instructions here?

Trying to infer from code, when navigating to /wp-admin/edit.php?post_type=wp_block, clicking the Import from JSON button causes the button to disappear, with a few errors in console:

Uncaught TypeError: Cannot set property isMounted of #<Component> which has only a getter
    at new ImportForm (index.js:23)
    at constructClassInstance (react-dom.24169eaf.js:12025)
    at updateClassComponent (react-dom.24169eaf.js:13722)
    at beginWork (react-dom.24169eaf.js:14402)
    at performUnitOfWork (react-dom.24169eaf.js:16441)
    at workLoop (react-dom.24169eaf.js:16480)
    at HTMLUnknownElement.callCallback (react-dom.24169eaf.js:140)
    at Object.invokeGuardedCallbackDev (react-dom.24169eaf.js:178)
    at invokeGuardedCallback (react-dom.24169eaf.js:227)
    at replayUnitOfWork (react-dom.24169eaf.js:15888)
ImportForm @ index.js:23
constructClassInstance @ react-dom.24169eaf.js:12025
updateClassComponent @ react-dom.24169eaf.js:13722
beginWork @ react-dom.24169eaf.js:14402
performUnitOfWork @ react-dom.24169eaf.js:16441
workLoop @ react-dom.24169eaf.js:16480
callCallback @ react-dom.24169eaf.js:140
invokeGuardedCallbackDev @ react-dom.24169eaf.js:178
invokeGuardedCallback @ react-dom.24169eaf.js:227
replayUnitOfWork @ react-dom.24169eaf.js:15888
renderRoot @ react-dom.24169eaf.js:16540
performWorkOnRoot @ react-dom.24169eaf.js:17138
performWork @ react-dom.24169eaf.js:17060
performSyncWork @ react-dom.24169eaf.js:17032
interactiveUpdates$1 @ react-dom.24169eaf.js:17297
interactiveUpdates @ react-dom.24169eaf.js:2326
dispatchInteractiveEvent @ react-dom.24169eaf.js:4882

react-dom.24169eaf.js:14804 The above error occurred in the <ImportForm> component:
    in ImportForm (created by WithInstanceId(ImportForm))
    in WithInstanceId(ImportForm) (created by Dropdown)
    in div (created by Popover)
    in div (created by Popover)
    in PopoverDetectOutside (created by WrappedPopoverDetectOutside)
    in WrappedPopoverDetectOutside (created by Popover)
    in Unknown (created by WithFocusReturn(Component))
    in div (created by WithFocusReturn(Component))
    in WithFocusReturn(Component) (created by WithConstrainedTabbing(WithFocusReturn(Component)))
    in div (created by WithConstrainedTabbing(WithFocusReturn(Component)))
    in WithConstrainedTabbing(WithFocusReturn(Component)) (created by Popover)
    in span (created by Popover)
    in Popover (created by Dropdown)
    in div (created by Dropdown)
    in Dropdown (created by ImportDropdown)
    in ImportDropdown

Consider adding an error boundary to your tree to customize error handling behavior.
Visit https://fb.me/react-error-boundaries to learn more about error boundaries.
logCapturedError @ react-dom.24169eaf.js:14804
logError @ react-dom.24169eaf.js:14843
update.callback @ react-dom.24169eaf.js:15496
callCallback @ react-dom.24169eaf.js:11456
commitUpdateQueue @ react-dom.24169eaf.js:11500
commitLifeCycles @ react-dom.24169eaf.js:14974
commitAllLifeCycles @ react-dom.24169eaf.js:16040
callCallback @ react-dom.24169eaf.js:140
invokeGuardedCallbackDev @ react-dom.24169eaf.js:178
invokeGuardedCallback @ react-dom.24169eaf.js:227
commitRoot @ react-dom.24169eaf.js:16181
completeRoot @ react-dom.24169eaf.js:17196
performWorkOnRoot @ react-dom.24169eaf.js:17141
performWork @ react-dom.24169eaf.js:17060
performSyncWork @ react-dom.24169eaf.js:17032
interactiveUpdates$1 @ react-dom.24169eaf.js:17297
interactiveUpdates @ react-dom.24169eaf.js:2326
dispatchInteractiveEvent @ react-dom.24169eaf.js:4882

react-dom.24169eaf.js:17121 Uncaught TypeError: Cannot set property isMounted of #<Component> which has only a getter
    at new ImportForm (index.js:23)
    at constructClassInstance (react-dom.24169eaf.js:12025)
    at updateClassComponent (react-dom.24169eaf.js:13722)
    at beginWork (react-dom.24169eaf.js:14402)
    at performUnitOfWork (react-dom.24169eaf.js:16441)
    at workLoop (react-dom.24169eaf.js:16480)
    at renderRoot (react-dom.24169eaf.js:16520)
    at performWorkOnRoot (react-dom.24169eaf.js:17138)
    at performWork (react-dom.24169eaf.js:17060)
    at performSyncWork (react-dom.24169eaf.js:17032)
Show outdated Hide outdated packages/list-reusable-blocks/src/index.js
Show outdated Hide outdated packages/list-reusable-blocks/src/index.js
}
const showNotice = () => {
const notice = document.createElement( 'div' );

This comment has been minimized.

@aduth

aduth Sep 13, 2018

Member

Could we render a <Notice /> component?

@aduth

aduth Sep 13, 2018

Member

Could we render a <Notice /> component?

This comment has been minimized.

@youknowriad

youknowriad Sep 14, 2018

Contributor

I did try it, the issue is that it's styling is very different from the regular notices in this kind of pages. We should reconsider if we rewrite the whole page in JS

@youknowriad

youknowriad Sep 14, 2018

Contributor

I did try it, the issue is that it's styling is very different from the regular notices in this kind of pages. We should reconsider if we rewrite the whole page in JS

const postType = await apiFetch( { path: `/wp/v2/types/wp_block` } );
const reusableBlock = await apiFetch( { path: `/wp/v2/${ postType.rest_base }/${ id }` } );
const fileContent = JSON.stringify( {
__file: 'wp_block',

This comment has been minimized.

@aduth

aduth Sep 13, 2018

Member

I'm trying to understand what this is for. Is this a marker of the file being of block format? Does it really matter? Or could we just infer by validity of the rest of the file (being JSON, having title, etc).

Or at least, "file" may not be a well-describing word here (vs. "format", "type", etc).

Also wondering if there's some prior art here to lean on without over-complicating (JSON-LD, JSON Schema, xmlns).

@aduth

aduth Sep 13, 2018

Member

I'm trying to understand what this is for. Is this a marker of the file being of block format? Does it really matter? Or could we just infer by validity of the rest of the file (being JSON, having title, etc).

Or at least, "file" may not be a well-describing word here (vs. "format", "type", etc).

Also wondering if there's some prior art here to lean on without over-complicating (JSON-LD, JSON Schema, xmlns).

This comment has been minimized.

@youknowriad

youknowriad Sep 14, 2018

Contributor

yes, this is just a marker. My idea is that this will remove any ambiguity while trying to auto-import this file (like if we implement drag and drop to the editor). We could detect that the imported file has a "title" and "content" strings (JSON Schema) but this seems too common, I prefer as always explicitness over implicitness. What if we add another exported file with the same format that should imported differently? (like in a File block or something).

I also considered adding a version field if ever we change the format but I decided against because we can think of a missing version as version: 0

@youknowriad

youknowriad Sep 14, 2018

Contributor

yes, this is just a marker. My idea is that this will remove any ambiguity while trying to auto-import this file (like if we implement drag and drop to the editor). We could detect that the imported file has a "title" and "content" strings (JSON Schema) but this seems too common, I prefer as always explicitness over implicitness. What if we add another exported file with the same format that should imported differently? (like in a File block or something).

I also considered adding a version field if ever we change the format but I decided against because we can think of a missing version as version: 0

This comment has been minimized.

@aduth

aduth Sep 14, 2018

Member

Another thought: Do we ever intend to support batch import / export? Would this file contain multiple at some point? Will this format support that as a future enhancement?

@aduth

aduth Sep 14, 2018

Member

Another thought: Do we ever intend to support batch import / export? Would this file contain multiple at some point? Will this format support that as a future enhancement?

This comment has been minimized.

@aduth

aduth Sep 14, 2018

Member

Explicitness is fine, just seems a format arbitrarily decided upon.

@aduth

aduth Sep 14, 2018

Member

Explicitness is fine, just seems a format arbitrarily decided upon.

Show outdated Hide outdated packages/list-reusable-blocks/src/utils/import.js
{
"name": "@wordpress/list-reusable-blocks",
"version": "1.0.0",
"description": "Adding Export/Import support to the reusable blocks listing.",

This comment has been minimized.

@aduth

aduth Sep 13, 2018

Member

Really stress-testing our policy of publishing all the things as modules, huh 😅

@aduth

aduth Sep 13, 2018

Member

Really stress-testing our policy of publishing all the things as modules, huh 😅

This comment has been minimized.

@youknowriad

youknowriad Sep 14, 2018

Contributor

I made it private :)

@youknowriad

youknowriad Sep 14, 2018

Contributor

I made it private :)

This comment has been minimized.

@youknowriad

youknowriad Sep 14, 2018

Contributor

Now that I think about it. Making it private means it can't be included in Core as an external package and begs the question of whether it should be moved to Core or kept in the repo post-merge.

@youknowriad

youknowriad Sep 14, 2018

Contributor

Now that I think about it. Making it private means it can't be included in Core as an external package and begs the question of whether it should be moved to Core or kept in the repo post-merge.

This comment has been minimized.

@aduth

aduth Sep 14, 2018

Member

I mean, I never said I was opposed to it being published to npm 😉

@aduth

aduth Sep 14, 2018

Member

I mean, I never said I was opposed to it being published to npm 😉

This comment has been minimized.

@youknowriad

youknowriad Sep 14, 2018

Contributor

yes, I know it was private even before the comment :)

@youknowriad

youknowriad Sep 14, 2018

Contributor

yes, I know it was private even before the comment :)

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 14, 2018

Member

Small usability thing: A bit disconcerting to see the success notice, but not the block that I'd just imported (until a refresh).

image

Member

aduth commented Sep 14, 2018

Small usability thing: A bit disconcerting to see the success notice, but not the block that I'd just imported (until a refresh).

image

@aduth

aduth approved these changes Sep 14, 2018

$actions['export'] = sprintf(
'<a class="wp-list-reusable-blocks__export" href="#" data-id="%s" aria-label="%s">%s</a>',
$post->ID,
__( 'Export as JSON', 'gutenberg' ),

This comment has been minimized.

@aduth

aduth Sep 14, 2018

Member

Is this convention lifted from somewhere? Doesn't seem obvious why we'd need an aria-label which has the same text as the element itself.

@aduth

aduth Sep 14, 2018

Member

Is this convention lifted from somewhere? Doesn't seem obvious why we'd need an aria-label which has the same text as the element itself.

This comment has been minimized.

@youknowriad

youknowriad Sep 14, 2018

Contributor

yes, it's inspired by the way we add the "Classic editor" link in the post list actions

@youknowriad

youknowriad Sep 14, 2018

Contributor

yes, it's inspired by the way we add the "Classic editor" link in the post list actions

const postType = await apiFetch( { path: `/wp/v2/types/wp_block` } );
const reusableBlock = await apiFetch( { path: `/wp/v2/${ postType.rest_base }/${ id }` } );
const fileContent = JSON.stringify( {
__file: 'wp_block',

This comment has been minimized.

@aduth

aduth Sep 14, 2018

Member

Another thought: Do we ever intend to support batch import / export? Would this file contain multiple at some point? Will this format support that as a future enhancement?

@aduth

aduth Sep 14, 2018

Member

Another thought: Do we ever intend to support batch import / export? Would this file contain multiple at some point? Will this format support that as a future enhancement?

reader.onload = function() {
resolve( reader.result );
};
reader.readAsText( file );

This comment has been minimized.

@aduth

aduth Sep 14, 2018

Member

I have to check browser support for this one to see if we need a polyfill.

Did you check? As best I can tell from browser support resources, it should be supported.

https://developer.mozilla.org/en-US/docs/Web/API/FileReader/readAsText#Browser_compatibility

@aduth

aduth Sep 14, 2018

Member

I have to check browser support for this one to see if we need a polyfill.

Did you check? As best I can tell from browser support resources, it should be supported.

https://developer.mozilla.org/en-US/docs/Web/API/FileReader/readAsText#Browser_compatibility

const postType = await apiFetch( { path: `/wp/v2/types/wp_block` } );
const reusableBlock = await apiFetch( { path: `/wp/v2/${ postType.rest_base }/${ id }` } );
const fileContent = JSON.stringify( {
__file: 'wp_block',

This comment has been minimized.

@aduth

aduth Sep 14, 2018

Member

Explicitness is fine, just seems a format arbitrarily decided upon.

@aduth

aduth Sep 14, 2018

Member

Explicitness is fine, just seems a format arbitrarily decided upon.

uiMessage = __( 'Invalid Reusable Block JSON file' );
break;
default:
uiMessage = __( 'Unknow error' );

This comment has been minimized.

@aduth

aduth Sep 14, 2018

Member

Typo: "Unknow" -> "Unknown"

@aduth

aduth Sep 14, 2018

Member

Typo: "Unknow" -> "Unknown"

@youknowriad youknowriad merged commit ff4bc70 into master Sep 14, 2018

2 checks passed

codecov/project 48.69% (-0.34%) compared to ee6d092
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@aduth aduth deleted the try/reusable-blocks-import-export branch Sep 14, 2018

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Sep 14, 2018

Contributor

A good puzzle to solve is going to be handling local resources (IDs, etc) through this process.

Contributor

mtias commented Sep 14, 2018

A good puzzle to solve is going to be handling local resources (IDs, etc) through this process.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 17, 2018

Member

Some usability issues with editing reusable blocks noted at #9964, made more apparent here with the introduction of the "Manage All Reusable Blocks" link.

Member

aduth commented Sep 17, 2018

Some usability issues with editing reusable blocks noted at #9964, made more apparent here with the introduction of the "Manage All Reusable Blocks" link.

@janicecchua

This comment has been minimized.

Show comment
Hide comment
@janicecchua

janicecchua Sep 19, 2018

Was able to export and import reusable blocks with no issues. Just one thing which is a bit inconvenient is you'd have to refresh the page after importing to show the imported block.

janicecchua commented Sep 19, 2018

Was able to export and import reusable blocks with no issues. Just one thing which is a bit inconvenient is you'd have to refresh the page after importing to show the imported block.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 21, 2018

Contributor

@janicecchua yes, that's true and that's a compromise we made for now. I talked about it above

I fixed the bug on import success. A compromise I made though is to avoid refreshing the page on a successful import. The idea is that later on, we probably would avoid the PHP generated page entirely so instead of battling and trying to find a way to properly refresh and show the notice, we should the success notice and at some point when the refactoring of this page is done to be full JS rendered, it would be easy to just refresh the listing.

Contributor

youknowriad commented Sep 21, 2018

@janicecchua yes, that's true and that's a compromise we made for now. I talked about it above

I fixed the bug on import success. A compromise I made though is to avoid refreshing the page on a successful import. The idea is that later on, we probably would avoid the PHP generated page entirely so instead of battling and trying to find a way to properly refresh and show the notice, we should the success notice and at some point when the refactoring of this page is done to be full JS rendered, it would be easy to just refresh the listing.

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