Add image banner height settings #695

kmeleta · 2021-09-28T15:13:37Z

Why are these changes introduced?

Fixes #552

What approach did you take?

Replaced fixed heights with setting specific values using a banner--<size> class.

Also included a banner box width update for widescreens.

Demo links

Editor

Checklist

melissaperreault

Thank you so much Ken!

Took a first bite at it and let's keep having a different max-height value for all the setting options on mobile!

Here is what I had in mind for mobile:

Small = 28rem
Medium = 34rem
Large = 39rem

I am continuing to investigate the info text language and the behaviour for when the setting Adapt section height to first image size is set to true.

melissaperreault · 2021-09-29T03:53:25Z

locales/en.default.schema.json

@@ -863,7 +876,8 @@
          "label": "Show text below images"
        },
        "adapt_height_first_image": {
-          "label": "Adapt section height to first image size"
+          "label": "Adapt section height to first image size",
+          "info": "Overwrites image banner height setting when checked."


We'll keep this for now but we know that moving this setting inside the Image banner height setting would be a better enhancement for merchants but would require a breaking change. Took a note about it over here to consider later.

kmeleta · 2021-09-29T16:15:40Z

Small = 28rem
Medium = 34rem
Large = 39rem

@melissaperreault mobile heights added, let me know how they look. I definitely think this works better than sharing the desktop values.

melissaperreault · 2021-09-29T20:22:21Z

Following our IRL chats, when testing the most up to date section settings including the one merged here #663, I have one last request to make to adjust the box max-width for wider screens.

For that scenario, when the box is displayed only on desktop, I tested 90rem and looks better for wide screen (Screen recording). When the box isn't displayed, the max-width is already good!

KimliW · 2021-09-30T17:48:42Z

@melissaperreault Can I get some clarification on this please? I need the heights for each setting (small medium large) for desktop and mobile for the Help Center docs.

kmeleta · 2021-09-30T20:18:13Z

Hey @KimliW, these are the heights for each of the setting options as it stands. The setting currently defaults to Medium

Tablet/Desktop:
Small: 420px
Medium: 560px
Large: 720px

Mobile:
Small: 280px
Medium: 340px
Large: 390px

melissaperreault

Thank you Ken! Looking real good! 🎉

sofiamatulis

Looks good 👍 Added small comments

sofiamatulis · 2021-10-04T15:00:30Z

assets/section-image-banner.css

@@ -277,6 +305,12 @@
  }
 }

+@media screen and (min-width: 1400px) {
+  .banner__box {
+    max-width: 90rem;


Was this PR rebased? Just want to ensure this change has the up to date changes from the width PR since I see a max-width added here.

Max width added on the other PR: https://github.com/Shopify/dawn/pull/663/files#diff-8a48cf1c4c77bd24f3d47ceac2c19b0149a25b33c5e4f3d3155d249448ffdcfaR268-R272

cc @melissaperreault

Yeah it should have been rebased onto that banner width pr. This change was a follow up to those changes for widescreens from Meli. I should have updated the pr description to mention that.

Correct, combined with the changes to the height created here, I saw an opportunity to polish the final outcome combined with the other PR 👌

sofiamatulis · 2021-10-04T15:01:11Z

sections/image-banner.liquid

+      ],
+      "default": "medium",
+      "label": "t:sections.image-banner.settings.image_height.label",
+      "info": "t:sections.image-banner.settings.image_height.info"


I believe youll need to request translations

For sure, I figure I should make sure the language added looks good to at least one dev before I request.

I believe it looks good! :) I would confirm with @melissaperreault and @gretchen-willenberg if there are any changes that need to be made before requesting them

We are good to go for now! Thanks! 🙏

ludoboludo

Couple comments but looked good to me.

ludoboludo · 2021-10-04T17:16:31Z

assets/section-image-banner.css

+  }
+
+  .banner--large:not(.banner--mobile-bottom):not(.banner--adapt) .banner__content {
+    min-height: 39rem;


those could maybe be grouped with the styling declaration above on line 8, 13, 18 ?

ludoboludo · 2021-10-04T17:29:36Z

assets/section-image-banner.css

@@ -161,10 +187,12 @@

  .banner__content--flex-start {
    align-items: flex-start;
+    padding-bottom: 15rem;


I wonder if we would run into any odd scenarios when the image is wider than its height. It would only zoom in a little so I don't think it's an issue 🤔

I am not sure I understand the use case @ludoboludo , do you mind testing it?

You bet, here is what I mean. Video

Agree with Ludo, when the "adapt" setting is enabled, the image should keep its intrinsic ratio. Unless textbox content overflows, in which case the height of the image should just grow with the textbox and the ratio will need to change.

Thank you Ludo for explaining! Agreed that we are ok! 👌

~~Although, I am noticing something that I want to test, if I remove the box, the text and buttons should be vertically centered on the banner. I might have missed this detail during my review. 🤔~~

edit: 😵‍💫 Forget what I said! We have a setting to set the content position on the banner that I forgot for 1 minute! 😅

ludoboludo · 2021-10-04T18:03:03Z

locales/en.default.schema.json

+          "options__3": {
+            "label": "Large"
+          },
+          "info": "For best results, use an image with a 2:3 aspect ratio. [Learn more](https://help.shopify.com/en/manual/shopify-admin/productivity-tools/image-editor#understanding-image-aspect-ratio)"


Also I wonder, is 2:3 the aspect ratio we recommend ? Not 3:2 ? Since this section seem to be more for landscape images rather than portraits.

Good catch Ludo, Aligned with reverting the numbers to 3:2

Might want to check if we did this mistake on other sections where we mention 2:3 🤔 Taking a note to look for later!

Looks like featured and main blog uses the same 2:3 note. If those also should be updated, we can probably handle them all in a follow up so we don't have to re-request translations here.

Aligned! And yes, they should both be 3:2 too.

Sounds good 👌 Follow up PR it is. It could maybe be added on the new section for the password page as well that Sofia is working on: #698 (comment)

melissaperreault

🚢 🎉

KaichenWang

Looks good! Left a suggestion + comment

KaichenWang · 2021-10-08T19:12:41Z

assets/section-image-banner.css

@@ -161,10 +187,12 @@

  .banner__content--flex-start {
    align-items: flex-start;
+    padding-bottom: 15rem;


Agree with Ludo, when the "adapt" setting is enabled, the image should keep its intrinsic ratio. Unless textbox content overflows, in which case the height of the image should just grow with the textbox and the ratio will need to change.

KaichenWang · 2021-10-08T20:01:28Z

assets/section-image-banner.css

@@ -5,25 +5,58 @@
 }

 @media screen and (max-width: 749px) {
-  .banner--mobile-bottom:not(.banner--stacked):not(.banner--adapt)
-    > .banner__media {
+  .banner--small.banner--mobile-bottom:not(.banner--adapt) > .banner__media,


These selectors for defining height may benefit from a refactor to ensure increased code readability and avoiding selector repetition. Something like this:

.banner--adapt { --image-banner-height: initial; --image-banner-height-desktop: initial; } .banner--small { --image-banner-height: 28rem; --image-banner-height-desktop: 42rem; } .banner--medium { --image-banner-height: 34rem; --image-banner-height-desktop: 56rem; } .banner--large { --image-banner-height: 39rem; --image-banner-height-desktop: 72rem; } @media screen and (max-width: 749px) { .banner--mobile-bottom > .banner__media, .banner--stacked:not(.banner--mobile-bottom) > .banner__media { height: var(--image-banner-height); } .banner:not(.banner--mobile-bottom) .banner__content { min-height: var(--image-banner-height); } } @media screen and (min-width: 750px) { .banner { flex-direction: row; min-height: var(--image-banner-height-desktop); } }

That's not a bad thought Kai. There may be some other code around that could benefit from a similar refactor as. I'll look into creating a cleanup PR

* Added new height settings * Update mobile height styles * Add mobile specific height values * Add wide screen max width for banner box * Update 8 translation files * Update 8 translation files * Update 1 translation file * Update 1 translation file * Update 1 translation file * Update 1 translation file Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>

kmeleta marked this pull request as ready for review September 28, 2021 15:36

kmeleta requested a review from melissaperreault September 28, 2021 15:36

melissaperreault reviewed Sep 28, 2021

View reviewed changes

sofiamatulis mentioned this pull request Sep 28, 2021

Password iteration #698

Merged

5 tasks

melissaperreault reviewed Sep 29, 2021

View reviewed changes

kmeleta added 3 commits September 29, 2021 09:55

Added new height settings

8af06e1

Update mobile height styles

304fb6e

Add mobile specific height values

b92d99c

kmeleta force-pushed the image-banner-height branch from ccaea9a to b92d99c Compare September 29, 2021 16:03

kmeleta requested a review from melissaperreault September 29, 2021 16:15

Add wide screen max width for banner box

e5b809f

melissaperreault previously approved these changes Sep 30, 2021

View reviewed changes

sofiamatulis reviewed Oct 4, 2021

View reviewed changes

ludoboludo self-requested a review October 4, 2021 16:35

ludoboludo previously approved these changes Oct 4, 2021

View reviewed changes

ludoboludo reviewed Oct 4, 2021

View reviewed changes

Update 8 translation files

ae989b6

translation-platform bot dismissed stale reviews from ludoboludo and melissaperreault via ae989b6 October 5, 2021 19:05

translation-platform bot added 5 commits October 6, 2021 03:04

Update 8 translation files

cded625

Update 1 translation file

a76b93b

Update 1 translation file

1cb66a3

Update 1 translation file

f76dd58

Update 1 translation file

13e9eee

sofiamatulis approved these changes Oct 8, 2021

View reviewed changes

melissaperreault approved these changes Oct 8, 2021

View reviewed changes

KaichenWang reviewed Oct 8, 2021

View reviewed changes

KaichenWang merged commit 844ed4a into main Oct 8, 2021

KaichenWang deleted the image-banner-height branch October 8, 2021 20:44

kmeleta mentioned this pull request Oct 21, 2021

Update aspect ratio language to 3:2 #813

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add image banner height settings #695

Add image banner height settings #695

kmeleta commented Sep 28, 2021 •

edited

Loading

melissaperreault left a comment •

edited

Loading

melissaperreault Sep 29, 2021

kmeleta commented Sep 29, 2021

melissaperreault commented Sep 29, 2021 •

edited

Loading

KimliW commented Sep 30, 2021

kmeleta commented Sep 30, 2021

melissaperreault left a comment

sofiamatulis left a comment

sofiamatulis Oct 4, 2021 •

edited

Loading

kmeleta Oct 4, 2021 •

edited

Loading

melissaperreault Oct 4, 2021

sofiamatulis Oct 4, 2021

kmeleta Oct 4, 2021

sofiamatulis Oct 4, 2021

melissaperreault Oct 6, 2021

ludoboludo left a comment

ludoboludo Oct 4, 2021

ludoboludo Oct 4, 2021

melissaperreault Oct 6, 2021

ludoboludo Oct 7, 2021

KaichenWang Oct 8, 2021

melissaperreault Oct 8, 2021 •

edited

Loading

ludoboludo Oct 4, 2021

melissaperreault Oct 6, 2021

melissaperreault Oct 6, 2021

kmeleta Oct 6, 2021

melissaperreault Oct 7, 2021 •

edited

Loading

ludoboludo Oct 7, 2021

melissaperreault left a comment

KaichenWang left a comment

KaichenWang Oct 8, 2021

KaichenWang Oct 8, 2021

kmeleta Oct 18, 2021

Add image banner height settings #695

Add image banner height settings #695

Conversation

kmeleta commented Sep 28, 2021 • edited Loading

melissaperreault left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmeleta commented Sep 29, 2021

melissaperreault commented Sep 29, 2021 • edited Loading

KimliW commented Sep 30, 2021

kmeleta commented Sep 30, 2021

melissaperreault left a comment

Choose a reason for hiding this comment

sofiamatulis left a comment

Choose a reason for hiding this comment

sofiamatulis Oct 4, 2021 • edited Loading

Choose a reason for hiding this comment

kmeleta Oct 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ludoboludo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melissaperreault Oct 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melissaperreault Oct 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melissaperreault left a comment

Choose a reason for hiding this comment

KaichenWang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmeleta commented Sep 28, 2021 •

edited

Loading

melissaperreault left a comment •

edited

Loading

melissaperreault commented Sep 29, 2021 •

edited

Loading

sofiamatulis Oct 4, 2021 •

edited

Loading

kmeleta Oct 4, 2021 •

edited

Loading

melissaperreault Oct 8, 2021 •

edited

Loading

melissaperreault Oct 7, 2021 •

edited

Loading