Revert "Cover: Fix matrix alignment issue." #28364
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
This reverts commit 1796afc.
ockham
added
[Type] Bug
jasmussen
approved these changes
Jan 20, 2021
|
Size Change: -5 B (0%) Total Size: 1.29 MB
ℹ️ View Unchanged
|
|
I created #28365. At this point I'm not sure whether we should land it as a point release, or get land this revert and look for the root cause that made the image shift around when using the matrix alignment. |
Thanks! I'm thinking to land the the revert and ship the point release with it, which basically gets us back to the state of Cover block styling prior to #28114, which is a good enough baseline -- mostly to derisk it, since at this point, I'm a bit wary that any other CSS changes might have other unwanted side effects. We can then merge #28365 to |
|
Hmm, really still isn't exactly pretty on the frontend:
Thinking of holding off on 9.8.1 a bit more to either include #28365 after all, and/or, ideally, find the root cause of the visual regression. @jasmussen would you have any spare cycles to look into the latter? 😊 |
|
That shift you show in the screenshot is what #28365 is meant to fix. I'm juggling some toddlers at the moment but can look later, and I believe what remains to be done is A) figure out when and how that matrix issue regressed in the first place |
|
In my highly un-scientific shotgun debugging session, as an alternative to bisecting, I went to the list of commits, went pages and pages back, checked out and old version and
First time I went far enough back, to December 15th, that the block was still using background images. In that version, it worked fine.
Then I checked out this changeset: #25171 and the bug was back again:
So I checked out the commit right before it, and the bug was gone.
So yes, it appears that we forgot to test the matrix alignment, when the Cover was refactored to use an image with srcsets instead. The take-away is that there wasn't ever a time after the srcset refactor, where the matrix alignment worked perfectly, so there isn't any mystical CSS regression to find. Which suggests we can keep exploring fixes like the one in #28365. And I still believe that fix is solid, simple, basic, and should work. Because of that, I'd recommend the next step be to test that thoroughly, then decide how and when to ship it. |
|
I have a branch that seems to be working fine before the revert: https://github.com/ajlende/gutenberg/tree/fix/cover-alignment edit: Opened it up as a PR #28404 |
Did you find out what was unique to that branch, making it work? I'm about to dive in and test your other branch. Thanks for all your work. |
Yep, and it's all in the PR already. |
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.
Reverts #28361
Reason: Visually broken on the frontend 😿
See #28361 (comment) for screenshots.