Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix repeated backgrounds for cover srcset #28310

Merged
merged 3 commits into from
Jan 20, 2021

Conversation

ajlende
Copy link
Contributor

@ajlende ajlende commented Jan 19, 2021

Description

Repeated backgrounds aren't working. That feature #26001 came along after this PR was made but got merged before it (#25171). I remember seeing it discussed #25421 (comment) that setting the Cover to have a fixed background is an exception to the img/srcset being used. So it would seem the repeated background option should trigger that same exception.

See #25171 (comment). Thanks @stokesman for reporting the bug.

This just adds a check for isRepeated in addition to hasParallax for using background styles.

How has this been tested?

  • Use the repeated background in the cover block. See that it still works.

  • Checkout cfe936a (commit before Add srcset for cover image #25171). Add the old block to a post and save the post. Checkout this branch. Re-open the post. See that the deprecated block gets upgraded.

Screenshots

Types of changes

Bug Fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@ajlende ajlende requested a review from mkaz January 19, 2021 21:50
@ajlende ajlende self-assigned this Jan 19, 2021
@ajlende ajlende added [Block] Cover Affects the Cover Block - used to display content laid over a background image [Type] Bug An existing feature does not function as intended labels Jan 19, 2021
@@ -152,7 +159,7 @@ const deprecated = [
);

return (
<div className={ classes } style={ style }>
<div { ...useBlockProps.save( { className: classes, style } ) }>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied this from the save function prior to the srcset change. The useBlockProps must have been added in addition to the isRepeated before the srcset change was merged.

Copy link
Member

@mkaz mkaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good and in testing I didn't find any issues.
👍

@ajlende ajlende merged commit 13e7ab3 into WordPress:master Jan 20, 2021
@ajlende ajlende deleted the fix/cover-srcset-repeated branch January 20, 2021 16:51
@github-actions github-actions bot added this to the Gutenberg 9.9 milestone Jan 20, 2021
@stokesman
Copy link
Contributor

stokesman commented Jan 20, 2021

So I'm just a little late to the party. I'd just captured a screen recording to post here when I saw it'd been merged. I would have recorded/posted earlier as I had tested this yesterday but I assumed what I saw was going to be caught by others.

Maybe I'm the only one seeing this but in my testing, this didn't appear ready. The repeated background is showing but also layered with the same image in the img element. The image I'm testing has transparency or else the repeated background would be obscured. Also, I forgot to capture it in the screen recording but the focal point picker only responds on vertical axis.

cover-srcset-repeated.mp4

It'd be great if there's just something going on with my setup and this is not reproducible.

@ajlende
Copy link
Contributor Author

ajlende commented Jan 20, 2021

@stokesman I must not have noticed because I had a large image without any transparency. I added a fix in #28362

ajlende added a commit to ajlende/gutenberg that referenced this pull request Jan 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Cover Affects the Cover Block - used to display content laid over a background image [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants