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

Update BlockPreview component to accept multiple Blocks to preview #16033

Merged
merged 14 commits into from
Jul 31, 2019

Conversation

marekhrabe
Copy link
Contributor

@marekhrabe marekhrabe commented Jun 7, 2019

Description

BlockPreview currently only accepts a single Block. This PR updates this to allow it to (optionally) accept an array of blocks. It also simplifies the API by requiring a Block object rather than expecting Block properties as individual args.

Note: the Reusable Blocks previews are not currently working due to this unrelated bug #16808

How has this been tested?

Firstly confirm existing behaviour has been preserved with the change in API. To do this, manually test all instances where BlockPreview is used and ensure all function as per master branch:

  • Insert Quote block, open Block Switcher, see Previews render
  • Open Block Inserter, scroll to Reusable Blocks, hover to see a preview (please see note above)

To test multiple Blocks being rendered you can:

  • Apply this diff: https://gist.github.com/obenland/761894e1a5be40091e41214756b6a323
    (Applying the diff is necessary to make this PR's under-the-hood changes visible)
  • Create a Post with a lot of Blocks in it.
  • In a quote or button block, open the Block switcher to see all of the Blocks from the page appear in the BlockPreviewContent below the standard Block Style previews.

test

Places to check for previews

  • Undo the additional diff from instructions above and test with code in this branch:
    • Block Inserter - top left of the screen the "plus" button. In the inserter, hover a reusable block (you might need to create one first) and you should see a preview on right. As noted above, you will see an error there. If you see an error, that means previews still work the same way and this PR didn't break it. Error is good here. Seeing nothing would be bad.
    • Block Switcher - for Button or Quote block it shows "Block Styles" as described above. Make sure both the thumbnail and the detail after hovering the thumbnail contain a preview of the style.

Types of changes

New feature (non-breaking change which adds functionality)

To Demo

Show multiple blocks being previewed.

@marekhrabe marekhrabe added the [Type] Enhancement A suggestion for improvement. label Jun 7, 2019
@marekhrabe marekhrabe requested a review from youknowriad June 7, 2019 09:37
@marekhrabe marekhrabe self-assigned this Jun 7, 2019
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Right now that component expects name, attributes and innerBlocks prop to render the preview. I think if we want to expose it, it would be better to allow passing blocks as an array instead of these granular props. What do you think?

@marekhrabe
Copy link
Contributor Author

It definitely sounds like a nicer API. Do you think we should update all current usages to use blocks or just add blocks as another way of using this component?

@youknowriad
Copy link
Contributor

Do you think we should update all current usages to use blocks

I'd opt for this. It seems straightforward?

@youknowriad
Copy link
Contributor

Also, I think we might want to expose BlockPreviewContent using the BlockPreview name instead. It seems that the current BlockPreview adds an unnecessary "title" that might not be useful for all usage. Thoughts?

This might require some adjustements to classnames and things like that.

@getdave
Copy link
Contributor

getdave commented Jun 17, 2019

Let's keep this in sync with #16113

cc @jasmussen

@getdave getdave changed the title Export BlockPreview component (3P) Export BlockPreview component Jun 17, 2019
@marekhrabe
Copy link
Contributor Author

marekhrabe commented Jun 17, 2019

I did some investigation during the weekend and I'm currently thinking between:

  1. exporting BlockPreview (previously known as BlockPreviewContent), and BlockPreviewFullSize(previously BlockPreview)
  2. replacing both usages with a single BlockPreview component which would have a prop that controls whether it's scaled (and how much) and I'd remove the "Preview" heading completely from our component, making sure it gets moved to places where BlockPreview is used from

The smaller size is used in "Styles" panel for example and the full size can be seen inside the block switcher or in block inserter for reusable blocks

I'm personally leaning towards the latter

@getdave
Copy link
Contributor

getdave commented Jul 3, 2019

@marekhrabe I've found the two-component modal pretty confusing. I'd lean towards number 2 also.

@marekhrabe
Copy link
Contributor Author

I also liked the approach when doing some prototypes locally, it was just a bit more work than expected as those two components do very different things and all places they are used from have to be updated.

With the rate #16113 is getting worked on and introducing a lot of changes, I think it might be wise to wait with this PR for that one to be merged. Otherwise, we will be doing some duplicate work about scaling etc…

@marekhrabe
Copy link
Contributor Author

marekhrabe commented Jul 30, 2019

I'm working separately on the unification of BlockPreview & BlockPreviewContent (WIP #16801).

With @getdave and @retrofox, we plan to spend some more time on various issues around block previews. To be able to ship gradual updates to these components, I would suggest to keep this PR only about the move from name, attributes, innerBlocks into blocks and make sure all preview usages will be migrated to blocks. I'd suggest removing the export from this PR.

Let's finish the support for blocks here, scaling in #16113 and write docs for the preview and export the component as the last thing. Would this make sense?

@retrofox
Copy link
Contributor

I've been testing the preview for the components that have defined the styles.

master branch
image

this branch

image

Although showing the + button into the preview already happens in prod, I wonder why it looks darker than in the master branch

@marekhrabe marekhrabe force-pushed the add/export-for-block-preview branch from 8b77521 to d04cf21 Compare July 30, 2019 23:57
@marekhrabe
Copy link
Contributor Author

I've done a rebase against the master branch (clean, no conflicts) so I can start integrating with #16801.

@marekhrabe marekhrabe force-pushed the add/export-for-block-preview branch from 97c05cb to 75f9bb0 Compare July 31, 2019 00:41
@marekhrabe
Copy link
Contributor Author

marekhrabe commented Jul 31, 2019

I have fixed the issue around innerBlocks being undefined which caused an avalanche of bugs in random places. #16033 (comment)

I think this is now ready for a review.

cc: @youknowriad you were suspicious about it and totally right!

@getdave
Copy link
Contributor

getdave commented Jul 31, 2019

I've been testing the preview for the components that have defined the styles.

master branch
image

this branch

image

Although showing the + button into the preview already happens in prod, I wonder why it looks darker than in the master branch

Suggest we filter this out if previews along with any other Block inserters.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM 👍Thanks for the updates.

I agree that the inserter on the left should be hidden on previews. for me it shows similarly (color) in both master and this branch.

@getdave getdave merged commit 96dcc4f into master Jul 31, 2019
@github-actions github-actions bot added this to the Gutenberg 6.3 milestone Jul 31, 2019
@obenland obenland deleted the add/export-for-block-preview branch August 8, 2019 16:01
gziolo pushed a commit that referenced this pull request Aug 29, 2019
…16033)

* export BlockPreview

* Update to accept multiple Blocks

* Update reusable Blocks preview to use the new single blocks prop

* Remove unecessary clone of Blocks

Not sure why this was introduced.

* Remove export. This is being handled in another PR

See #16801

* Simplify casting to array via lodash

* Utilise cloneBlocks to safely merge attributes on blocks prop

* Spread reusable block initial attrs correctly

* Fix cloneBlock fn to check for innerBlocks before attempting to map

The `innerBlocks` of the `block` being cloned can be `undefined`. Therefore, by attempting to map these we trigger an error.

Fixed to introduce existence check before attempting to manipulate innerBlocks.

* Simplify modifying passed block attrs via cloneBlock

* Removes unecessary spread operation

initalAttributes is already an object so no need to spread into an object.

* block-preview: ensuring to cast BlockEditorProvider value prop as an Array

* don't call cloneBlock on a non-block object

* bring back the old behavior of cloneBlock
gziolo pushed a commit that referenced this pull request Aug 29, 2019
…16033)

* export BlockPreview

* Update to accept multiple Blocks

* Update reusable Blocks preview to use the new single blocks prop

* Remove unecessary clone of Blocks

Not sure why this was introduced.

* Remove export. This is being handled in another PR

See #16801

* Simplify casting to array via lodash

* Utilise cloneBlocks to safely merge attributes on blocks prop

* Spread reusable block initial attrs correctly

* Fix cloneBlock fn to check for innerBlocks before attempting to map

The `innerBlocks` of the `block` being cloned can be `undefined`. Therefore, by attempting to map these we trigger an error.

Fixed to introduce existence check before attempting to manipulate innerBlocks.

* Simplify modifying passed block attrs via cloneBlock

* Removes unecessary spread operation

initalAttributes is already an object so no need to spread into an object.

* block-preview: ensuring to cast BlockEditorProvider value prop as an Array

* don't call cloneBlock on a non-block object

* bring back the old behavior of cloneBlock
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code Needs Technical Feedback Needs testing from a developer perspective. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants