Fix: ResponsiveWrapper can not handle cases with small dimensions #20892
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
jorgefilipecosta
added
the
[Type] Bug
jorgefilipecosta
requested review from
ajitbohra,
chrisvanpatten and
youknowriad
as code owners
|
Size Change: +36 B (0%) Total Size: 857 kB
ℹ️ View Unchanged
|
jorgefilipecosta
added
the
Backport to WP 6.7 Beta/RC
youknowriad
reviewed
Mar 16, 2020
| @@ -15,4 +15,5 @@ | |||
| left: 0; | |||
| width: 100%; | |||
| height: 100%; | |||
| margin: auto; | |||
There was a problem hiding this comment.
I guess this centers the small images, I'm not sure it's better than the left aligned images design-wise. That said, I don't have a strong preference.
There was a problem hiding this comment.
cc: @mapk to double-check if the design looks ok.
jorgefilipecosta
deleted the
fix/responsive-wrapper-does-not-handles-smaller-children
branch
|
I was able after a few tries to get into gutenberg.run to see the results of this fix. The fix looks better then what was seen in the #20697 Example after the fix.
God job! Thanks for tackling this issue, Jorge! |
aduth
reviewed
Mar 16, 2020
| if ( Children.count( children ) !== 1 ) { | ||
| return null; | ||
| } | ||
| const imageStyle = { | ||
| paddingBottom: ( naturalHeight / naturalWidth ) * 100 + '%', | ||
| paddingBottom: | ||
| naturalWidth < containerWidth |
There was a problem hiding this comment.
We may want to be conscious of the fact that containerWidth will be null the first time the component renders, and in comparisons like this, null is treated effectively the same as 0. I'm not sure based on the logic here if that's what we're expecting.
jorgefilipecosta
removed
the
Backport to WP 6.7 Beta/RC
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.
Fixes: #20697
Description
The ResponsiveWrapper component makes sure the elements (normally images) passed to it are resized to the available width and the proportions between width and height are kept.
The component has a bug and does not deal correctly with cases where the width of the children is smaller than the width available for the container.
It applied the following rule style "( naturalHeight / naturalWidth ) * 100 + '%'" this made the container, in a case where the height is 20 and the width is 10 it makes the height 2 times the width. This creates lots of unnecessary space:
We had this bug since the component was created but it was not very noticeable because we did not load thumbnails on the featured images, we loaded full-size images. In WordPress 5.4 we started loading thumbnails and this bug becomes very noticeable as noted in #20697 as almost all the images passed to the component are small.
This PR fixes the issue for small images when the width is smaller than the width of the container we simply make the height equal to the image height.
Screenshots
Before:
After: