Add Columns transform from Media & Text #29415
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stokesman
added
[Block] Columns
stokesman
commented
Feb 28, 2021
| { | ||
| type: 'block', | ||
| blocks: [ 'core/media-text' ], | ||
| isMatch: ( { mediaId } ) => !! mediaId, |
There was a problem hiding this comment.
This was intended to not offer the transform when no media has been selected. I couldn't get that to work (even just returning false here) and have left it here to inquire about it. Any ideas @ntsekouras? We could remove it. As of now, I made the transform default to inserting an empty Image block when transforming before any media has been selected.
There was a problem hiding this comment.
That's a tricky one :). The problem here is that isMatch is checked in a different flow than the actual transformation. It is checked in getPossibleBlockTransformations that is used to return the available block types to show in Block Switcher list.
After that check that has found Columns to be an eligible match (with 2 transformations though :) ) the actual transformation selection happens in a different place here:
gutenberg/packages/blocks/src/api/factory.js
Line 441 in 5cd4f3f
which selects the first match which happens to be the first declared one. You can check that if you put your new transform below the existing one, it will always choose the old one.
The wildcard * complicates things on how to combine other transforms. Recently I looked into a related problem here: #29296, with no clear path forwards for now.
--edit
After digging a bit more it seems that the prioritizations of transforms have been implemented a long ago here: #5701
So by adding a priority: 1 prop in your transform will return the wanted transformation.
There was a problem hiding this comment.
You can check that if you put your new transform below the existing one, it will always choose the old one.
True, I had noticed that as I started this with the new transform below the existing. I've pushed another commit to put it back below and use priority. I still couldn't get isMatch to prevent the transform but it's unnecessary for this PR as I think it is always preferable to use this transform even if no media has been selected.
Thanks for looking into this!
|
Size Change: +3.08 kB (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
ntsekouras
reviewed
Mar 2, 2021
There was a problem hiding this comment.
Thanks for working on this @stokesman!
It seems that with priority prop your problem will be solved. I think it's okay to transform in 2 columns even if no Image is set.
ntsekouras
reviewed
Mar 2, 2021
Indeed, it still seems better than the generic transform. The small rub was not knowing the type of media block to insert. We could not insert a media block for those conditions but 🤷♂️ . |
ntsekouras
approved these changes
Mar 4, 2021
There was a problem hiding this comment.
Thanks for your work here @stokesman! 💯
I pushed a small addition to pass the text and link colors as well, as you did with backgroundColor. Green CI and let's 🚢
|
Good catch @ntsekouras, thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds the ability to transform a Media & Text block into a Columns block with two Columns and all the relevant fixings from the original block.
Motivation
Since #26185 it has been possible to “transform” any single block into a Columns block. It's a generic operation that just wraps the block inside a single column Columns block. It seems unlikely to be the expected behavior when applied to a Media & Text block.
How has this been tested?
Manually transforming various Media & Text blocks and verifying any applicable attributes are transferred as expected.
Screenshots
columns-from-media-text.mp4
Types of changes
New feature
Checklist: