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

Unify BlockPreview & BlockPreviewContent into one component #16801

Merged
merged 18 commits into from
Aug 2, 2019

Conversation

marekhrabe
Copy link
Contributor

@marekhrabe marekhrabe commented Jul 29, 2019

Description

This PR updates the way BlockPreviewBlockPreviewContent are used. We want to soon make previews available for plugin developers and these two components were quite confusing together as seen in #16033 (comment).

This PR introduces a new version of BlockPreview which is used for both scaled and full-size previews (controlled by a prop isScaled) and allows you to add custom className to the preview wrapper for further styling if needed.

BlockPreviewContent is gone.

The boolean prop isScaled is a solution that simulates how the component is currently used in the codebase. It will be soon replaced with different way of doing scaling that is being cooked in #16113. For now, I think it's a good solution that will keep our current usages working the same way while paving the way for the new scaling API and exporting the component for devs to use in plugins.

How has this been tested?

  • in Block Inserter - hover any reusable block. A new "panel" with title "Preview" and the preview itself should show up.
  • focus some block with styles (button or quote for example):
    • check the block settings sidebar and expand "Block Styles" panel. It should contain previews
    • open the block switcher and check both thumbnails and also the full preview after hover which should match the styling of the one in block inserter

Screenshots

This is <BlockPreview isScaled blocks={ [ … ] } /> inside Block Styles:

Screenshot 2019-07-31 at 11 47 42

This is the same preview inside the block switcher. The "panel" on the right was moved from the BlockPreview component directly into places where it is being used from (Inserter and Swicther). BlockPreview component is now always only the previewed content and nothing else.

Screenshot 2019-07-31 at 13 53 11

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@marekhrabe
Copy link
Contributor Author

marekhrabe commented Jul 31, 2019

TODO: Block Inserter and Block Switcher both use the "preview on hover" fly-out panel but currently don't share CSS for it in my PR. Let's figure out how to do it (without backing that directly into BlockPreview like before).

I'll probably make a wrapper component for BlockPreview which will add the "panel" styling and be used it in both places.

I made BlockPreviewDropdownWrapper for this. I'm still wondering if it is the right way.

It is used this way:

<BlockPreviewDropdownWrapper>
  <BlockPreview blocks={ [ … ] } />
</BlockPreviewDropdownWrapper>

but I was wondering whether to made it into something more like HOC which will take the same props as BlockPreview and will pass them internally to the preview. The alternative usage could be something like:

<BlockPreviewForDropdowns blocks={ [ … ] } />

@marekhrabe marekhrabe changed the base branch from master to add/export-for-block-preview July 31, 2019 01:00
@marekhrabe
Copy link
Contributor Author

Rebased against #16033

getdave pushed a commit that referenced this pull request Jul 31, 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
@marekhrabe marekhrabe force-pushed the update/unify-block-preview-components branch from 097bc0f to 3251ebd Compare July 31, 2019 17:22
@marekhrabe marekhrabe changed the base branch from add/export-for-block-preview to master July 31, 2019 18:39
@marekhrabe
Copy link
Contributor Author

I think this is now ready for a review. Components work well for me, as described in the testing instructions.

@marekhrabe marekhrabe marked this pull request as ready for review July 31, 2019 21:29
@marekhrabe marekhrabe removed the [Status] In Progress Tracking issues with work in progress label Jul 31, 2019
@marekhrabe marekhrabe requested a review from mcsf July 31, 2019 22:22
@retrofox
Copy link
Contributor

retrofox commented Aug 1, 2019

but I was wondering whether to made it into something more like HOC which will take the same props as BlockPreview and will pass them internally to the preview. The alternative usage could be something like:

<BlockPreviewForDropdowns blocks={ [ … ] } />

I like the idea of combining the blocks:

<BlockPreviewDropdownWrapper>
  <BlockPreview blocks={ [  ] } />
</BlockPreviewDropdownWrapper>

However, maybe it worths create the new <PopoverAside /> component to make it usable not only for the BlockPreview one but other components.

@retrofox
Copy link
Contributor

retrofox commented Aug 1, 2019

I made BlockPreviewDropdownWrapper for this. I'm still wondering if it is the right way.

Forgot to mention that if we want to expose the <BlockPreview /> we will have to expose this one as well, right?

@marekhrabe
Copy link
Contributor Author

marekhrabe commented Aug 1, 2019

I don't mind it to be duplicated in the caller if necessary.

This was my concern and that's why I've introduced the DropdownWrapper to abstract the styling. Knowing this, I've reverted my last commit and we now have a copy of the "aside" in both Inserter and Switcher.

I think all feedback is now addressed here.

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.

I think we may have lost a border around the previews in the change here but tbh I don't think it's worse. It may be better design-wise.

@shaunandrews
Copy link
Contributor

I think we may have lost a border around the previews in the change here but tbh I don't think it's worse. It may be better design-wise.

That was intentional :) Glad you noticed. That border just felt extraneous and was so subtle it was barely noticeable anyways.

@marekhrabe marekhrabe merged commit 1322523 into master Aug 2, 2019
@github-actions github-actions bot added this to the Gutenberg 6.3 milestone Aug 2, 2019
@mcsf mcsf deleted the update/unify-block-preview-components branch August 7, 2019 10:09
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
* export BlockPreview

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

* Remove export. This is being handled in another PR

See #16801

* mark all usages with red for easier visual tracking

* make a new unified component

* use new component in style thumbnail

* update usage in block switcher, add isScaled & className support

* revert initial state in block switcher

* use in block inserter

* use the new unified component as default, migrate usages

* drop word 'unified' from classname

* remove extra import. h/t @retrofox

* remove border around previews

* fix styling of scaled previews

* make abstract component for styling previews inside dropdowns

* move block switcher & inserter to use common component for styling

* drop the old editor-block-preview class

* revert dropdown component abstraction

This reverts commit 5bc673e.
This reverts commit 3006c01.
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
* export BlockPreview

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

* Remove export. This is being handled in another PR

See #16801

* mark all usages with red for easier visual tracking

* make a new unified component

* use new component in style thumbnail

* update usage in block switcher, add isScaled & className support

* revert initial state in block switcher

* use in block inserter

* use the new unified component as default, migrate usages

* drop word 'unified' from classname

* remove extra import. h/t @retrofox

* remove border around previews

* fix styling of scaled previews

* make abstract component for styling previews inside dropdowns

* move block switcher & inserter to use common component for styling

* drop the old editor-block-preview class

* revert dropdown component abstraction

This reverts commit 5bc673e.
This reverts commit 3006c01.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants