Preserve bindings metadata in block transforms #59179
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
|
Size Change: -52 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 5e3eda0. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7987644150
|
SantosGuillamot
requested review from
kevin940726,
Mamaduka and
ajitbohra
as code owners
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
SantosGuillamot
requested review from
gziolo,
ntsekouras and
michalczaplinski
SantosGuillamot
added
[Type] Bug
42 tasks
|
Thanks for putting the PR together!
IMO, the The
The I have wondered if there's an automatic way to transform the binding attributes, for example passing in an object like Some blocks also add a new wrapper (e.g. paragraph to group/columns), so there are some interesting permutations. It might be difficult to handle this completely automatically. The maintenance of handling every block individually is also quite a lot to consider though 😄 |
gziolo
reviewed
Feb 20, 2024
gziolo
reviewed
Feb 20, 2024
|
First of all, major props to @SantosGuillamot for proposing the initial implementation so spark the discussion.
The same issue exists for block attributes. Since block bindings depend on them, I don't think we can automate in any way without doing the same for the attributes themselves.
I think the biggest challenge is that In effect, I would be in favor of making the transformation explicit for now and make it coded in the cases where it makes sense today. In the future, if we repeat something in all places, we can abstract it away. |
|
This is looking great for the initial rollout after the iterations applied from my perspective. I left one #59179 (comment) regarding replicating the approach fro the Button block on other modified blocks. |
|
I gave this a test and it's all working as expected for me, and nothing jumps out at me regarding the util logic 👍 I've tried rerunning the failing mobile tests and they failed again — I'm not sure why |
Good catch! It looks like the code for Gutenberg Mobile has some overridden files that don't return the new helper method. My recommendation would be to move the util to |
This makes sense to me. I believe mobile tests are failing because I didn't expose the function in the private-apis.native.js file. EDIT: I changed it in this commit. |
gziolo
added
the
Backport to WP 6.7 Beta/RC
gziolo
approved these changes
Feb 21, 2024
|
In this commit, I changed the util trying to understand which properties should be preserved based on the block supports. The goal was to make it easier to use. This way, you just have to pass the name of the original and the resulting blocks. I believe this should help to abstract it further in the future. Let me know what you think. I'll add some tests to the code block as well. EDIT: Code block transform tests added. |
gziolo
reviewed
Feb 21, 2024
|
I believe I implemented all the changes I had in mind. I will work on supporting block renaming in other block transform in a follow-up PR. |
gziolo
reviewed
Feb 21, 2024
gziolo
approved these changes
Feb 21, 2024
|
Thanks for the PR! This is an interesting challenge here and this seems like a first iteration that actually kind of centralizes the handling, even not being a generic solution. I'm definitely going to check it more when I'm back(currently in a team meetup). |
|
Thanks for jumping in 🙂
Would you like us to wait until you review it or should we merge it and work on improvements in follow-up PRs? |
I have checked the code and tested already, and others have reviewed. Of course you can land :) Great job here @SantosGuillamot ! |
getdave
pushed a commit
that referenced
this pull request
Feb 27, 2024
* Add metadata in heading transforms * Add metadata in button transforms * Add heading and button transform from/to paragraph * Revert "Add heading and button transform from/to paragraph" This reverts commit c3138e3. * Move tests to individual block specs * Explicitly transform metadata in button block * Explicitly transform metadata in heading block * Make bindings transform explicit in heading block * Use util to transform metadata * Use metadata util in code block to keep id and name * Move util to block-library package * Change code transforms util path * Inherit transform metadata props in the util * Add transform tests for code block * Return early if no supported properties * Use bindings callback for mapping attributes
|
I just cherry-picked this PR to the cherry-pick-wp-6-5-beta-3 branch to get it included in the next release: 6a8bf9d |
getdave
added
Backported to WP Core
9 tasks
getdave
pushed a commit
that referenced
this pull request
Feb 27, 2024
* Add metadata in heading transforms * Add metadata in button transforms * Add heading and button transform from/to paragraph * Revert "Add heading and button transform from/to paragraph" This reverts commit c3138e3. * Move tests to individual block specs * Explicitly transform metadata in button block * Explicitly transform metadata in heading block * Make bindings transform explicit in heading block * Use util to transform metadata * Use metadata util in code block to keep id and name * Move util to block-library package * Change code transforms util path * Inherit transform metadata props in the util * Add transform tests for code block * Return early if no supported properties * Use bindings callback for mapping attributes
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.
What?
Fixes #59086
This pull request adds the possibility of preserving and adapting the metadata attribute for the following transforms:
I've also added some tests to check that, in those transformations, the content, align, and metadata attributes are preserved. I couldn't find any existing tests for that. Please let me know if that's not the case.
A couple of questions that come to my mind:
metadataor only the bindings?Why?
Right now, if you rename your block, or add a binding to it, and transform it into another block, that data is not preserved.
How?
I just added some logic in the heading, and the button transforms objects to pass the
metadataattribute as well.Testing Instructions
Test renaming a block is preserved
Repeat the process transforming a heading into a paragraph and a paragraph into a button.
Test bindings in a block are preserved
Repeat the process transforming a heading into a paragraph.
Repeat the process transforming a paragraph into a button. In this case, the object should change to point to the "text" attribute instead of "content".