Add transform between cover and media & text #38458
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
walbo
added
[Type] Enhancement
|
Size Change: -446 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
apeatling
requested changes
Mar 28, 2022
There was a problem hiding this comment.
These transforms make a lot of sense. Nice work! 👍
Like @apeatling, I had mixed results when a text color was selected. It appears the main issue is one block opts into color support for text and the other relies on its inner blocks to provide that customization.
Transforming a Media & Text block to a cover block loses the color selection immediately for me. If instead of setting the text color on the Media & Text block, you set it on the inner paragraph, then the color is maintained given the transform doesn't modify the inner blocks.
Yeah, I'm not sure how to solve this since the cover block uses the |
I think it would be difficult to fix this without refactoring the Cover block to have a text color option as well as overlay color, and it would make sense to have it opt into the color block supports for this. If we were to do this I think that would be a separate PR to this one. It seems like this is a small enough limitation to be able to merge this PR knowing this, and knowing that it will be followed up later once Cover has text color support? |
@glendaviesnz there could be another interim option to this, which I've been exploring this morning. We could have the transform propagate the text color selections onto the inner blocks. If the inner blocks don't opt into color support it has no effect however if the inner block does opt in, the user's color selections are maintained. It needs more testing and further thought but it could reduce the impact of the text color issue and allow these transforms to land sooner. I'll put together a separate PR extending this one so it can be discussed further in isolation. |
|
As per my last comment, I've created #40666 to demo the possibility of applying text color selections to inner blocks. It appears to work Media & Text > Cover > Media & Text. When converting back the inner blocks retain the color selections rather than them being moved back to the Media & Text block, I think this is ok for now. Interested to hear what everyone thinks, in particular, about honoring the user made color selections over the To move the addition of these transforms forward, I see a few options:
|
|
I don't think we should hold off because I think this is a beneficial transform even with the loss of text color. I noticed that your PR raised some issues, if we can resolve those then this combined with that change seems like a good approach. |
I'm not sure we can make the UX perfect when transforming Media & Text > Cover > Media & Text. I didn't find a means of identifying when we copied the text color onto the inner block versus when the user had set them on the inner block in the initial Media & Text. That said, it appears we're all leaning towards getting these transforms merged. Mostly likely with #40666 applied first. If the Cover block eventually opts into text color block support, the transform can be simplified then. @walbo are you happy with the above plan? Any thoughts or concerns? |
|
@aaronrobertshaw Sound good to me. Added a comment on #40666 |
aaronrobertshaw
approved these changes
May 3, 2022
There was a problem hiding this comment.
I've merged in the changes from #40666 and given this another test.
Looks to be working well for me though it might be good to get another approval from @apeatling or @glendaviesnz given the most recent changes made to this PR were mine.
22 tasks
15 tasks
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.
Description
Add transform between cover and media & text
Testing Instructions
Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.jsfiles for terms that need renaming or removal).