Add image banner settings #900

KaichenWang · 2021-11-17T17:23:50Z

Why are these changes introduced?

Fixes #890

What approach did you take?

Add setting to schema for Text alignment
Add options to existing setting Desktop text position to enable horizontal positioning
Clean up CSS (especially related to banner button layout)
Rely on applying setting values directly as CSS property values, instead of relying on adding them as element class
modifiers (See "Other considerations")

Other considerations

Before
Use setting value to set element class modifier. Then use CSS to target class modifiers.

/* Liquid */

banner__content--{{ section.settings.desktop_text_box_position }}

/* CSS */

.banner__content--center {
  align-items: center;
}

.banner__content--flex-start {
  align-items: flex-start;
}

.banner__content--flex-end {
  align-items: flex-end;
}

After
Use setting value directly in CSS. This requires less code overall if there are multiple options.

/* Liquid style tag */

#Banner-{{ section.id }} .banner__content {
  align-items: {{ section.settings.desktop_text_box_position | split: " " | first }};
  justify-content: {{ section.settings.desktop_text_box_position | split: " " | last }};
}

Demo links

Store
Editor

Checklist

KaichenWang · 2021-11-17T22:12:22Z

assets/section-image-banner.css

+.banner__buttons {
+  display: inline-flex;
+  flex-wrap: wrap;
+  gap: 1rem;


We may need to use margin instead, since flex gap isn't supported in all latest (and latest -1) browser versions. Although the previous implementation already uses this property https://github.com/Shopify/dawn/pull/900/files#r751679171

Something here to keep in mind / double check 🤔

Gap is being used across the theme in many other files 🤔 maybe we can make an audit

Sounds good. To note is that the gap property is well supported for grid layout but less for flexbox ones.

Created an issue here :) #964

KaichenWang · 2021-11-17T22:14:04Z

assets/section-image-banner.css

-.banner__box > .banner__buttons {
-  display: flex;
-  align-items: baseline;
-  gap: 1rem;


sofiamatulis · 2021-11-18T19:45:47Z

I will be taking over this section now :)

sofiamatulis · 2021-11-19T17:31:41Z

We now have settings for mobile text alignment as well.

https://screenshot.click/19-31-0q6f8-wk4nx.png

@wiktoriaswiecicka Not sure if this should be under mobile layout?

ludoboludo

Looks good. Just noticed an issue with button alignment.

sections/image-banner.liquid

assets/section-image-banner.css

sections/image-banner.liquid

assets/section-image-banner.css

sofiamatulis · 2021-11-24T17:45:05Z

The content has been updated:

updated order
replaced text with content
updated default values

cc @wiktoriaswiecicka @ludoboludo

sofiamatulis · 2021-11-25T17:41:26Z

assets/section-email-signup-banner.css

@@ -1,3 +1,36 @@
+.email-signup-banner .newsletter-form,


@melissaperreault @wiktoriaswiecicka What do you think of updating the label for the mobile alignment? Or maybe it's clearer to keep it 🤔

Original comment

#916 (comment)

Aligned it shouldn't say mobile under the "mobile" header.

I am late to the party! But as discussed in today's UX review, we'll keep "mobile" under mobile header for clarity.

sofiamatulis · 2021-11-25T18:16:16Z

Updated both sections email banner and image banner to have text style setting:

https://screenshot.click/25-15-4nx4s-9rbj1.png

cc @wiktoriaswiecicka @melissaperreault

martinamarien · 2021-12-07T15:29:57Z

assets/section-image-banner.css

+@media screen and (min-width: 750px) {
+  .banner__content {
+    padding: 5rem;
+  }
+
+  .banner__content--top-left {
+    align-items: flex-start;
+    justify-content: flex-start;
+  }
+
+  .banner__content--top-center {
+    align-items: flex-start;
+    justify-content: center;
+  }
+
+  .banner__content--top-right {
+    align-items: flex-start;
+    justify-content: flex-end;
+  }
+
+  .banner__content--middle-left {
+    align-items: center;
+    justify-content: flex-start;
+  }
+
+  .banner__content--middle-center {
+    align-items: center;
+    justify-content: center;
+  }
+
+  .banner__content--middle-right {
+    align-items: center;
+    justify-content: flex-end;
+  }
+
+  .banner__content--bottom-left {
+    align-items: flex-end;
+    justify-content: flex-start;
+  }
+
+  .banner__content--bottom-center {
+    align-items: flex-end;
+    justify-content: center;
+  }
+
+  .banner__content--bottom-right {
+    align-items: flex-end;
+    justify-content: flex-end;
+  }
+}


This needs to go below the .banner__content class below. Otherwise the setting doesn't work on desktop because it's always overwritten by the value below.

martinamarien

I found one issue and a setting we need to update. I know Ludo has already approved, so we can merge it but we need to make a PR right after to fix address two things:

The desktop content position section isn't working for both sections.
text_style alignment. I think we should remove this for the email signup section. We can touch base with Wik and Meli, but if the plan is to remove this for this setting type, I'd like to remove it before the release this week.

martinamarien · 2021-12-07T15:45:09Z

sections/email-signup-banner.liquid

+            {
+              "value": "caption-with-letter-spacing",
+              "label": "t:sections.email-signup-banner.blocks.paragraph.settings.text_style.options__3.label"
+            }


I think we are going to remove this style from text blocks
Do we want to wait until we add the caption block to remove it? If so, I think we should increase the size of the uppercase font because it's 10px currently.
cc. @melissaperreault @wiktoriaswiecicka

martinamarien · 2021-12-07T17:32:26Z

assets/section-email-signup-banner.css

+
+  .banner--desktop-transparent .email-signup-banner__box--no-image .field__button:focus-visible {
+    outline: 0.2rem solid rgba(var(--color-base-text), 0.5);
+    outline-offset: 0.3rem;


I was just suggesting removing the outline-offset: 0.3rem; line, as it's directly assigned to all elements that can receive :focus-visible. I know we will need the Outline and box-shadow properties 👍🏻

KaichenWang added 4 commits November 16, 2021 16:12

Update settings and schema

6d76881

Adjust conditional class selectors and add section inline styles

3b412f8

Clean up CSS

ee603f8

Add new line

de6381d

KaichenWang added the Category: Enhancement New feature or request label Nov 17, 2021

KaichenWang requested a review from wiktoriaswiecicka-zz November 17, 2021 17:23

KaichenWang closed this Nov 17, 2021

KaichenWang reopened this Nov 17, 2021

ghost added the cla-needed label Nov 17, 2021

KaichenWang commented Nov 17, 2021

View reviewed changes

assets/section-image-banner.css

.banner__box > .banner__buttons {

display: flex;

align-items: baseline;

gap: 1rem;

Copy link

Contributor Author

KaichenWang Nov 17, 2021

Choose a reason for hiding this comment

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

sofiamatulis self-assigned this Nov 18, 2021

sofiamatulis mentioned this pull request Nov 18, 2021

Add email banner settings #916

Merged

5 tasks

Add mobile align

3ee9e6d

ludoboludo reviewed Nov 19, 2021

View reviewed changes

sections/image-banner.liquid Outdated Show resolved Hide resolved

sections/image-banner.liquid Outdated Show resolved Hide resolved

assets/section-image-banner.css Show resolved Hide resolved

sofiamatulis requested a review from ludoboludo November 22, 2021 16:34

ludoboludo reviewed Nov 22, 2021

View reviewed changes

sections/image-banner.liquid Outdated Show resolved Hide resolved

assets/section-image-banner.css Outdated Show resolved Hide resolved

sofiamatulis requested a review from ludoboludo November 22, 2021 20:02

ludoboludo reviewed Nov 22, 2021

View reviewed changes

assets/section-image-banner.css Outdated Show resolved Hide resolved

assets/section-image-banner.css Outdated Show resolved Hide resolved

sofiamatulis requested a review from ludoboludo November 23, 2021 14:44

martinamarien self-requested a review November 24, 2021 18:16

sofiamatulis assigned ludoboludo and martinamarien Nov 24, 2021

sofiamatulis reviewed Nov 25, 2021

View reviewed changes

sofiamatulis requested a review from melissaperreault November 25, 2021 18:27

sofiamatulis and others added 11 commits December 3, 2021 14:26

feedback

a9995fd

New values

f5d0948

Rename desktop layout values

1f46e7c

fix typo

b287bf2

Fix formatting

a0cb526

Fix formatting

f5757f2

Fix formatting

92ffbcd

typo

ad9d51c

conflict

d4e4f5a

fix conflict

37c93bb

Update locales/vi.schema.json

1a8e78f

sofiamatulis force-pushed the add-image-banner-settings branch from cc56bbb to 1a8e78f Compare December 3, 2021 19:26

ghost removed the cla-needed label Dec 3, 2021

sofiamatulis added 2 commits December 3, 2021 15:04

cleanup

0fe668e

feedback

3d8cfa2

sofiamatulis requested a review from martinamarien December 3, 2021 20:23

fix typo

f3ce56e

sofiamatulis mentioned this pull request Dec 3, 2021

Email banner not showing content on mobile #967

Closed

wiktoriaswiecicka approved these changes Dec 3, 2021

View reviewed changes

ludoboludo approved these changes Dec 3, 2021

View reviewed changes

martinamarien reviewed Dec 7, 2021

View reviewed changes

martinamarien approved these changes Dec 7, 2021

View reviewed changes

sofiamatulis merged commit 8cf0d7b into main Dec 7, 2021

sofiamatulis deleted the add-image-banner-settings branch December 7, 2021 19:23

sofiamatulis mentioned this pull request Dec 7, 2021

Banner followup #975

Closed

5 tasks

sofiamatulis pushed a commit that referenced this pull request Dec 8, 2021

Add image banner settings (#900)

04c4b3d

sofiamatulis mentioned this pull request Dec 8, 2021

Banner/Email banner follow ups #990

Merged

5 tasks

DaveEdwards97 mentioned this pull request Dec 21, 2021

[Image Banner] Additional desktop text box placements and alignments #801

Closed

phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024

Add image banner settings (Shopify#900)

6d1c749

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add image banner settings #900

Add image banner settings #900

KaichenWang commented Nov 17, 2021

KaichenWang Nov 17, 2021 •

edited

Loading

ludoboludo Dec 2, 2021

sofiamatulis Dec 2, 2021

ludoboludo Dec 2, 2021

sofiamatulis Dec 3, 2021

KaichenWang Nov 17, 2021

sofiamatulis commented Nov 18, 2021

sofiamatulis commented Nov 19, 2021

ludoboludo left a comment

sofiamatulis commented Nov 24, 2021

sofiamatulis Nov 25, 2021

melissaperreault Nov 26, 2021

katycobb Dec 2, 2021

sofiamatulis commented Nov 25, 2021

martinamarien Dec 7, 2021

martinamarien left a comment

martinamarien Dec 7, 2021

martinamarien Dec 7, 2021

Add image banner settings #900

Add image banner settings #900

Conversation

KaichenWang commented Nov 17, 2021

KaichenWang Nov 17, 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

sofiamatulis commented Nov 18, 2021

sofiamatulis commented Nov 19, 2021

ludoboludo left a comment

Choose a reason for hiding this comment

sofiamatulis commented Nov 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sofiamatulis commented Nov 25, 2021

Choose a reason for hiding this comment

martinamarien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KaichenWang Nov 17, 2021 •

edited

Loading