Try: Cover height fix round 2. #28455
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: -12 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
|
I took this for a spin and didn't find any issues. Tried themes twenty-nineteen up to twenty-twenty-one. It appears that the focal point picker issue encountered when previously removing the |
Excellent point, thank you for chiming in! 💯 |
|
Thank you for the follow through on this @jasmussen! I tested this and the focal picker doesn't seem to be playing nicely. With this PR checked out:
If it's helpful, I verified this on master as well with the following steps:
|
jasmussen
force-pushed
the
try/cover-height
branch
from
d8f06dc to
775d156
Compare
|
Thank you for testing, @jffng. But even after rebasing, I can't reproduce your issue, and I'm not sure why. The focal point picker is working for me both in Chrome, Firefox, Safari, with portrait and landscape images. I am seeing an issue with the focal point picker not being able to go left or right, in portrait images, but that appears to be a separate issue that's being discussed in #28487. |
|
Here's a GIF:
|
jasmussen
force-pushed
the
try/cover-height
branch
from
775d156 to
1aee649
Compare
|
I gave this another spin and the issue with the focal point picker I reported above is fixed. This LGTM, thanks @jasmussen ! |
|
Thank you Alex! I'd also love a sanity check from @brentswisher as I know he's looked at adjacent things. |
brentswisher
approved these changes
Feb 2, 2021
There was a problem hiding this comment.
This looks good to me too. I'm not seeing any issues with the focal point picker in my testing. I actually don't think many of those issues were caused by the CSS changes, it just brought them to our attention. I'm hoping to be able to add some automated tests to the focal point component in the near future so making changes isn't quite so hectic!
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.
Alternative to #28114. Maybe fixes #17921, #17919.
The problem: The Cover block has some compounding rules around height, that make it rather ill-mannered when inside a flex container. Specifically, the
height: 100%;makes the height incorrect when inside a flex container, taking up the full height regardless of what you specifically set it to using the input field in the sidebar. That effectively pushes content off the bottom of the flex container, making it overlap adjacent content.Before:
After:
In previous attempts, a few things broke because of this:
All three appear to work just fine, with removing the height now:
So, what's different from last time we tried the exact same PR?
@ockham and @ajlende have tirelessly been refactoring the adjacent code to be more resilient. Specifically the new rules added around how the image is positioned inside the cover block, appear to have solved this. In other words, the
height: 100%;has been removed from the cover block itself, to theimginside it.How to reproduce the before and after:
Paste the following into your code editor:
Initially my instinct was that this was a bug with the columns block, because after all, ideally it should be able to contain any block you throw inside it, right? But my realization was that no matter how resilient you make a container block, the CSS of a child block can make it explode if you try. In this case, the height property truly was causing issues.
To test:
Let's take it slow with this one. It's a good fix to land, but it doesn't have to happen quickly. Let's test it thoroughly, and if it passes the checks, merge it early in a cycle.
It would be good to test this branch on:
Things to look for:
As far as I can tell, this one is solid. But let's be sure!
CC: @jffng and @stokesman because you worked on an adjacent focal point picker issue.