Show Cover block placeholder only if it has no inner blocks #31402
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: +19 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
priethor
added
[Block] Cover
|
Thanks so much for your work here! This is before:
This is after:
As is hopefully evident here, the experience of clearing media is vastly improved here, and it does indeed fix the two tickets mentioned. However there's a recent "update background media" feature that regressed in this PR, that we should strive to keep if we can. Here's what happens if you drag and drop an image onto a cover block in trunk:
Here's after:
There's a conversation to be had whether you should be able to both replace the image background, in addition to uploading an Image block as an inner-block of a Cover — in trunk you can only do the former at the moment. Perhaps that could be solved by drag/dropping only along the edge of the Cover, whereas if you drag in the center, it becomes an innerblock. But that seems a separate conversation, and that we should probably keep the behavior in trunk at least for now. |
stokesman
force-pushed
the
update/cover-block-placehold
branch
from
ab1b3e4 to
77ec62f
Compare
|
Thanks so much for testing and finding that regression @jasmussen. It was easy enough to not break it but my first attempt here was a bit obtuse 😅 . I've pushed another commit to get it working again. |
jasmussen
approved these changes
May 5, 2021
There was a problem hiding this comment.
Very nice, works well for me!
I'd love a sanity check if someone has time to give one, but this is an improtant fix, it's relatively simple code, looks good, and I'd suggest by the law of diminishing returns, it'd be good to merge after a bit if no-one else chimes in. Thank you!
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.
Intended to resolve the apparent disappearance of content when the background is removed (to fix: #31253 and fix: #28888)
How has this been tested?
Manually by adding and removing backgrounds in Cover blocks
Screenshots
It can be seen that removing backgrounds does not return to the placeholder state. An edge case this doesn't work well for can be seen toward the end of the video. If the content color happens to match the fallback background color the content cannot be seen once the background is removed.
cover-block-less-placeholdery.mp4
Types of changes
Bug fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).