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

[Image with Text] Fixed background position #2530

Closed
wants to merge 2 commits into from

Conversation

LucasLacerdaUX
Copy link
Contributor

@LucasLacerdaUX LucasLacerdaUX commented Apr 14, 2023

PR Summary:

Introduces a new Fixed background position option to Image behavior in the Image with Text section.

Why are these changes introduced?

Fixes #2344.

What approach did you take?

  • Added the setting.
  • Leverage the existing animation--{{ image_behavior }} class to apply animation--fixed
  • Uses styles from [Image behavior] Fixed background position #2523
  • Use fade-in animation when using fixed position (position:fixed is not compatible with transform, as it creates a new positioning context by design)

Testing steps/scenarios

  • Add Image with Text section.
  • Set Image behavior to Fixed background position
  • Set an image
  • Test with Image first and Image second values for Desktop image placement
  • Test with placeholder only
  • Test on mobile and desktop

Demo links

Checklist

@LucasLacerdaUX LucasLacerdaUX marked this pull request as ready for review April 26, 2023 17:05
@melissaperreault melissaperreault self-requested a review April 26, 2023 17:14
@melissaperreault
Copy link
Contributor

Note: Really doesn't work well with landscape images 😬

@LucasLacerdaUX
Copy link
Contributor Author

Good catch. My bad. I'll give this some extra tests and tweaks, @melissaperreault.

It's a bit tricky to get the sizes right on this one as it's only taking a small portion of the page but the image below needs a fixed width. We have ways to know whether an image is landscape or portrait by its aspect ratio, so there is probably a way to make some changes to treat them differently on CSS.

Base automatically changed from fixed-position-background to main May 1, 2023 14:45
@LucasLacerdaUX LucasLacerdaUX force-pushed the fixed-position-img-w-text branch from c6cd0df to d1adaea Compare May 23, 2023 14:56
@LucasLacerdaUX LucasLacerdaUX force-pushed the fixed-position-img-w-text branch from d1adaea to 8d5a1ee Compare June 12, 2023 15:27
@ludoboludo
Copy link
Contributor

I kinda feel like we could question the usage of position: fixed; on this section.
The way a fixed element works doesn't seem to align with how the image with text section works.

Fixed images tend to take up all the viewport space and the image from the image with text section isn't always that big. You can think of its container as the window through which we can see the image taking the full viewport size. So what we can see if very limited to the size of that "window", kinda like this:

Unless there is a workaround.

Right now, only when the image is set to large height and width it will make sense to enable it 🤔 And on mobile as well since the image will be 100% width of the viewport.

What's your thoughts on this @LucasLacerdaUX ?

@LucasLacerdaUX
Copy link
Contributor Author

Update: we decided to pause the development of this feature for now.

What needs to be done

  • The transform: perspective(0) we introduced in Gradient fix for transparent background medias and cards #2651 needs to be reverted for this container if we want to proceed with this feature. The transform property prevents the position: fixed background from working as expected, as it will create a new container layer by CSS specs.
  • We need to ensure images work well for portrait and landscape images.
    • The current width and left/right positioning does not work well with all image aspect ratios across different viewport sizes.
    • We may have to use image.aspect_ratio to detect whether it's a portrait or landscape image, and apply different strategies for both.
  • Provide clear instructions so merchants can provide a high-resolution image. By default, any position: fixed element will take 100% of its container size (in this case, the viewport) and we're simply cropping the displayed area so it's closer to the focal point.
    • We need to either add a label or make it clear in the documentation that the image displayed with this property on WILL be different and needs a much higher resolution (100% viewport) rather than something that fits the image container in this section.
    • We need to update our own img_sizes applied to image_tag to serve viewport-sized images, similar to an image banner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Image with Text] Add fixed position background
3 participants