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 Banner] Fixed background position #2524

Merged
merged 9 commits into from
May 1, 2023

Conversation

LucasLacerdaUX
Copy link
Contributor

@LucasLacerdaUX LucasLacerdaUX commented Apr 13, 2023

PR Summary:

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

Why are these changes introduced?

Fixes #2343.

What approach did you take?

Testing steps/scenarios

  • Add Image Banner section.
  • Set Image behavior to Fixed background position
  • Test with 1 and 2 images
  • Test with placeholder only
  • Test on mobile and desktop

Demo links

Checklist

@LucasLacerdaUX LucasLacerdaUX force-pushed the fixed-position-background branch from ff72b70 to 2e89412 Compare April 13, 2023 18:35
@LucasLacerdaUX LucasLacerdaUX force-pushed the fixed-position-img-banner branch from c6186f6 to 270bb25 Compare April 13, 2023 18:35
@LucasLacerdaUX LucasLacerdaUX linked an issue Apr 13, 2023 that may be closed by this pull request
left: 50%;
width: 50%;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't seem to manage the focal point that well on mobile and is really zooming in on the image: video

I think it's specifically an issue on sections that are at the top of the page or very bottom (if the footer is small in terms of height).

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually it's even more so specific to the admin when you view the mobile layout. Because you have a pretty tall browser window. When I test from my phone it's better because it's as tall. Could be an odd experience for merchants to understand 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

@ludoboludo Can you record a video to explain what you observe? I am not sure I understand 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the video shared in my first message shows what I'm observing. Basically the thing to know here is that a fixed element is placed based on the viewport itself not the element its in.

So there can be some poor experiences in certain scenarios:

  • if the viewport height is quite high
  • if you're viewing the site on big screen/wide browser window as the image has a max size of 3840px wide
  • depending on the image format, portrait/landscape, some example are more or less obvious
  • if the image/section is at the bottom/top of the page, the visible part isn't going to show what you're actually trying to show in some occasions

Here is a video recap of those examples (it's pretty long 😬 7min)

Copy link
Contributor Author

@LucasLacerdaUX LucasLacerdaUX Apr 20, 2023

Choose a reason for hiding this comment

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

About focal points, I'm actually not sure what the solution would be here. In order for this effect to work for every section position, the height of the image must be 100vh, otherwise you will have empty space at some point while scrolling.

Here's an example where I added max-height: 50% (arbitrary value) to limit the image heights:

20-36-p6eac-g7t4z.mp4

It looks perfect for the first section, but as you scroll, you realize that the images MUST be 100% height for the effect to work properly. I would say the is almost an expected side-effect of turning this setting on: it will cause the focal point to not show up by default in some situations. Should we have any help text to let merchants know of that? If you have any alternative suggestions to solve this issue, let me know.

As for image sizes, I think you just made a great point in the video that we may need to consider bumping the resolution we serve them. I'm pretty sure that right now it's serving a lower quality image than we should, especially on mobile where it's 50% area but shown with a 100% zoom image.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ludoboludo I've updated the image sizes to 100% so there is no resolution drop. You can see a clear improvement in mobile and half-width images.

Here's the before & after:

imageimage

image

image

@melissaperreault melissaperreault self-requested a review April 20, 2023 04:06
Copy link
Contributor

@melissaperreault melissaperreault left a comment

Choose a reason for hiding this comment

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

Thank you Lucas!

Didn't find anything apart from what Ludo found! Otherwise it looks good! 🙌

LucasLacerdaUX and others added 2 commits April 26, 2023 11:54
Co-authored-by: Ludo <ludo.segura@shopify.com>
Copy link
Contributor

@ludoboludo ludoboludo left a comment

Choose a reason for hiding this comment

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

Looking good. A couple comments.

I wonder if we should add some info in the editor or help docs about the impact on performance if you do too many fixed background on the page 🤔

Copy link
Contributor

@ludoboludo ludoboludo left a comment

Choose a reason for hiding this comment

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

Nitpicks, otherwise looks good 👍

LucasLacerdaUX and others added 2 commits April 28, 2023 16:22
Co-authored-by: Ludo <ludo.segura@shopify.com>
Co-authored-by: Ludo <ludo.segura@shopify.com>
@LucasLacerdaUX LucasLacerdaUX merged commit 63f3ba4 into fixed-position-background May 1, 2023
@LucasLacerdaUX LucasLacerdaUX deleted the fixed-position-img-banner branch May 1, 2023 14:38
phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024
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 banner] Add fixed position background
4 participants