Maintain text color transforming from media to cover #40666
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
aaronrobertshaw
added
[Type] Enhancement
aaronrobertshaw
commented
Apr 28, 2022
Comment on lines
+176
to
+181
| // Only apply the media and text color if the inner block | ||
| // doesn't set its own color block support selection. | ||
| if ( | ||
| innerBlock.attributes.textColor || | ||
| innerStyle?.color?.text | ||
| ) { |
There was a problem hiding this comment.
We might also be able to check for color block support on the inner blocks.
|
Size Change: +150 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
8 tasks
|
Nice! ... a minor issue I can see is that a user can set the It is a compromise either way. I don't have a strong opinion about which compromise will cause the least grief, but I suspect more people would be confused by the fact that the Media & Text text color selection no longer works for them than those that wondered why the text color vanished and didn't just apply it again via the parent - but purely a hunch on my part. |
Are users more likely to undo rather than re-transform back?
I'd lean towards not losing user selections as the priority. The number transforming from Media and Text to Cover block would be higher than the number transforming and then transforming back (lessened still by those using undo). The number experiencing an issue with the color now being on the child block would be only the further subset that wish to tweak the color again after the fact. My hunch is that the greater impact is the initial loss of color selection on the first transform. |
🤷♂️ ... I am probably just a bit wary of doing anything that might add further complexity to the Cover block, but actually, this doesn't affect the Cover block in any way that would require future deprecations to unwind, so I am easy either way. |
Agree. |
|
Thanks for the additional PR. I do think it makes a better user experience to not loose the color. A bit confusing that it will change location if you transform back and forth, but I belive this is a compromise we will have to make as long as the cover block doesn't have text color on the block. The plus side is that we can change the behavor in the future without deprecations if the cover block adds it. So a +1 from my side. |
walbo
approved these changes
Apr 29, 2022
walbo
pushed a commit
that referenced
this pull request
May 9, 2022
* Add transform between cover and media & text * Add align * Remove use of set * Maintain text color transforming from media to cover (#40666) Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>
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.
Related:
What?
Extends the Media & Text to Cover transform introduced in #38458 to apply any text color selection from the Media & Text block to the inner blocks moved to the new Cover block.
Why?
This aims to more faithfully keep user color selections when transforming from Media & Text blocks to Cover blocks.
How?
textColororstyle.color.textvalue if the inner block doesn't already have its own color block support selections.Testing Instructions
Some example block code pre transforms
Screenshots or screencast
Screen.Recording.2022-04-28.at.12.07.40.pm.mp4