Fix site logo unresized size take 2 #30846
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
jasmussen
added
[Type] Regression
7 tasks
|
Size Change: +1.86 kB (0%) Total Size: 1.46 MB
ℹ️ View Unchanged
|
ockham
reviewed
Apr 15, 2021
| @@ -337,6 +337,7 @@ export default function LogoEdit( { | |||
| ); | |||
|
|
|||
| const classes = classnames( className, { | |||
| 'is-default-size': ! width, | |||
| 'is-resized': !! width, | |||
There was a problem hiding this comment.
Do we still need this then? Seems like there's only one occurence left here:
gutenberg/packages/block-library/src/site-logo/editor.scss
Lines 13 to 15 in 82ddc9e
...which we could probably replace with a &:not(.is-default-size)?
There was a problem hiding this comment.
The is-resized rule, and the table display, was present before the changes I made to default sizes, and I believe are still necessary. I'd prefer to not touch them, or at least touch them separately if need be.
I suspect they were ported over when the block was copied from its Image block roots, and in the image block the table property is used to size the image the same as the caption when manually sized. But I'd still like to fix that separately.
There was a problem hiding this comment.
Oh, I didn't mean to remove the rule entirely, but rather to change its selector to &:not(.is-default-size) so we can drop the now-redundant is-resized className 😄
There was a problem hiding this comment.
(If we can't do that, I think I'd prefer #30845, since it doesn't introduce another class name that is essentially just is-resized negated, which is kinda redundant.)
There was a problem hiding this comment.
Oh of course! I'll get on it. Can I have 20 minutes? I have to pick up a kid very quick
There was a problem hiding this comment.
Okay here we go!
Editor:
Frontend:
Seemes to work fine! Thank you Bernie!
ockham
reviewed
Apr 15, 2021
There was a problem hiding this comment.
Thanks for the fix! This seems to work overall. One thing I've noticed however is that for the _un_resized Site Logo, the aspect ratio isn't preserved on the frontend for me (this is with TT1 Blocks v0.4.5) -- which looks pretty weird for big images, since they're shrunk to 120px wide, but retain their original height.
Any chance we can fix that too? 😅
|
I added height auto, which of course should be there if the width is set. Thank you! |
ockham
approved these changes
Apr 15, 2021
ockham
pushed a commit
that referenced
this pull request
Apr 15, 2021
Add a new "is-default-size" class, and attach the 120px width to it.
|
Cherry-picked to |
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.
Description
Alternative to #30845. This one adds a new "is-default-size" class, to which it attaches the 120px width.
How has this been tested?
Insert a site logo block. Add a big image. Duplicate the block
One of the blocks you resize, the other you leave alone. Editor and frontend should look the same.
Checklist:
*.native.jsfiles for terms that need renaming or removal).