Implement Fixed image and Repeated image media controls for the Featured Product
#6344
Conversation
albarin
force-pushed
the
add/6235-bg-repeated-image
branch
from
f126738 to
8498c27
Compare
|
Size Change: +518 B (0%) Total Size: 873 kB
ℹ️ View Unchanged
|
albarin
added
type: enhancement
albarin
changed the title
Fixed image and Repeated image media controls for the Featured Product
albarin
force-pushed
the
add/6235-bg-repeated-image
branch
2 times, most recently
from
75dc0e4 to
dd9338b
Compare
|
While testing I have encountered this weird behavior: I have an image that's relatively smaller than its container. I change the background to “Fixed” and the focal point is reset to the center. However, the image gets thrown to the top left corner. Clicking around the focal point picker, allows me to restore the image location. Screen.Recording.2022-05-05.at.17.58.45.movAnother issue:
I expect the image to be stretched so it covers the available container. However, it gets stretched much larger. |
|
Thanks for the review @sunyatasattva
I just pushed a commit that I think fixes this issue.
Now that you mention it kind of makes sense, I followed the same behavior that the |
There was a problem hiding this comment.
I was testing more on this, and I noticed that neither Fixed nor Repeated options work with duoTone filters. I have no idea if there is a way to make them work, honestly. But we should do some research on that and then perhaps follow up with someone to see what to do about it. We can either:
- Not implement fixed/repeated options
- Find a workaround that's not too performance intensive
- Add a notice after the settings about duotone not working
- Disabling duotone entirely from the toolbar when either of those options are active
Now that you mention it kind of makes sense, I followed the same behavior that the Cover block has when selecting Fixed image, not sure what's the correct one. 🤔
Is there an easy way to try out stretching the image only until it covers the container? Otherwise, I'll leave the decision on this up to you. While testing several cases, this seemed really weird behavior for me, but I'm open to your opinion.
Good catch! I just pushed a couple of commits to fix it. I've tested all the cases I could think of and everything seems to be working as before the fix 🤞
I've been researching a bit about it and it seems there's an issue with |
albarin
force-pushed
the
add/6235-bg-repeated-image
branch
from
ab27159 to
d071d8a
Compare
There was a problem hiding this comment.
Duotone is fixed! Nice job! I'm approving, as I don't want to be an absolute pain on this issue, and everything works correctly.
However, the background issue is still bothering me a bit. Your link was an excellent find to explain what was happening.
I played around a bit, and I don't think it's worth adding JS to fix this issue as we can have a big impact on the performance of the page (we don't know how many of these blocks are people going to add in one page after all). What do you think?
While playing around I thought of a solution that could be viable, if you want to experiment a bit.
So, the issue in a nutshell is that when you tell a background to be fixed, its context will be calculated with the entire viewport: so the cover instruction essentially means to cover the entire viewport and not the wrapper container.
So, one way around it, would be to emulate the behavior of cover ourselves. So, instead of using background-size: cover, we calculate the dimensions of the container and hardcode that in the background-size (similarly to what we do with background-position).
Yes, this is a JS solution, but it's not a very expensive one, as we only need to do it as the user is editing the block, and then the front end should be able to render everything correctly via the PHP, causing no performance issues.
Oh, and last thing, again optional as I don't want to block this PR any longer: when choosing Fixed background option, the tag is transformed to a div, making the alt attribute useless. When I check Repeated background the alt option disappears from the sidebar, but it doesn't when I used the fixed one.
Both of these comments are optional, feel free to merge it if you are happy with the current state. None of it is a big deal.
|
@sunyatasattva I've hidden the alt textarea for Fixed background, good catch 👍 I've been trying your idea for cover but couldn't make it fully work, so I think for now we can proceed with this PR as is now, I don't want to delay it even more. Thanks for the reviews! |
|
Thank you for trying! I don't even know if it was a good idea. ¯_(ツ)_/¯ I've already approved the PR so you can proceed with the merge as far as I'm concerned. |
Fix error rebasing master
When isRepeated is true, the image is a background so it does not make sense to have an alt attribute.
albarin
force-pushed
the
add/6235-bg-repeated-image
branch
from
578e665 to
b6e4bf2
Compare
This PR adds the
Fixed imageandRepeated imagemedia controls for theFeatured Productblock.Fixes part of #6235
Testing
How to test the changes in this Pull Request:
Featured Product.Media Settingsand toggleFixed imageandRepeated imageand save.Fixed image,Repeated imageand any other configuration.Changelog