Add padding controls to the announcement bar #1789

kjellr · 2022-06-14T13:08:04Z

PR Summary:

Adds padding controls to the announcement bar block.

Why are these changes introduced?

Greater design flexibility.

What approach did you take?

This mirrors the approach taken for padding controls on sections (e.g. Rich Text).

Other considerations

Not sure if it makes sense to simplify down to just one single control here, instead of two. I think most merchants will likely want equal padding on the top and bottom.
The padding here was originally set using rem values, and it feels a little weird to move that back to pixel values. We could use rem values in the slider, but I don't believe we do that anywhere else.

Testing steps/scenarios

Open this test store
Adjust the padding on the announcement bar using the new controls.

Checklist

Added PR summary for release notes
Notified the help.shopify.com team about updates to theme settings (current contact: Kimli)
Followed theme code principles
Linted with Theme Check
Tested on mobile
Tested on multiple browsers
Tested for accessibility

kjellr · 2022-06-14T13:09:51Z

sections/announcement-bar.liquid

@@ -1,12 +1,26 @@
 {%- for block in section.blocks -%}
  {%- case block.type -%}
    {%- when 'announcement' -%}
+      {%- style -%}


I'm not totally sure about this part — it seems weird to declare this inline here, but that appears to be how it's handled for the other sections examples I've seen? I might be misinterpreting...

Yeah you're right with that approach 👍
Because we're looping through blocks and they each need their own styling, it makes sense to do it here and the way you did.

I think there could be an approach maybe using CSS custom properties 🤔
Having a general declaration referencing a custom property which is different/updated per blocks.
But I wonder if it would be beneficial. It could prevent outputting the whole declaration for every block but it's not like merchant use 27 announcement bars.

kjellr · 2022-06-14T13:10:30Z

sections/announcement-bar.liquid

+        },
+        {
+          "type": "header",
+          "content": "t:sections.all.padding.section_padding_heading"


This is "Section Padding" — I'm not sure if we need to declare a new string for "Block Padding" instead.

I kinda feel like it would either need to be named differently or only apply to the container/section for now.

Otherwise it's introducing a pattern that doesn't quite match the rest.

We could have a look at offering that same option of padding on blocks but across the theme 🤔

Thanks. I've moved these strings to be block-specific in f83ebdc. Let me know if you think that's a better solution for right now.

I think maybe we can plan ahead and make it a general translation string as well. As it's likely that we might add more of those block padding options in the future.

So I would just add a new heading here:

ludoboludo · 2022-06-17T15:53:33Z

sections/announcement-bar.liquid

@@ -1,12 +1,26 @@
 {%- for block in section.blocks -%}
  {%- case block.type -%}
    {%- when 'announcement' -%}
+      {%- style -%}


Yeah you're right with that approach 👍
Because we're looping through blocks and they each need their own styling, it makes sense to do it here and the way you did.

ludoboludo · 2022-06-17T16:09:19Z

sections/announcement-bar.liquid

@@ -1,12 +1,26 @@
 {%- for block in section.blocks -%}
  {%- case block.type -%}
    {%- when 'announcement' -%}
+      {%- style -%}


I think there could be an approach maybe using CSS custom properties 🤔
Having a general declaration referencing a custom property which is different/updated per blocks.
But I wonder if it would be beneficial. It could prevent outputting the whole declaration for every block but it's not like merchant use 27 announcement bars.

sections/announcement-bar.liquid

ludoboludo · 2022-06-17T16:17:28Z

sections/announcement-bar.liquid

+        },
+        {
+          "type": "header",
+          "content": "t:sections.all.padding.section_padding_heading"


I kinda feel like it would either need to be named differently or only apply to the container/section for now.

Otherwise it's introducing a pattern that doesn't quite match the rest.

We could have a look at offering that same option of padding on blocks but across the theme 🤔

ludoboludo

Looks good, I would just tweak the translation part and then we can request translations 👍

ludoboludo · 2022-06-30T21:24:31Z

sections/announcement-bar.liquid

+          "label": "t:sections.announcement-bar.blocks.announcement.settings.padding.padding_top",
+          "default": 10
+        },
+        {
+          "type": "range",
+          "id": "padding_bottom",
+          "min": 0,
+          "max": 100,
+          "step": 2,
+          "unit": "px",
+          "label": "t:sections.announcement-bar.blocks.announcement.settings.padding.padding_bottom",


I think you can go back to what you had before here so: t:sections.all.padding.padding_top and t:sections.all.padding.padding_bottom

Cool, thanks! Should be all set now and ready for a re-review.

This reverts commit f83ebdc.

ludoboludo

Looking good 👍 Ill request translations and re approve when they're all back in

translation-platform · 2022-07-05T07:35:16Z

We have received a translation request but there is a question blocking translation work. Your help is needed.

Click the following links to answer the questions

⚠️ Replying directly to this comment in GitHub will not work! 🙅‍♀️

Could you please clarify what exactly 'block padding' means? Thank you.. Answer here.

dawn/locales/en.default.schema.json

Line 337 in 009eecf

"block_padding_heading": "Block padding",

Not your issue?

Forward the issue to someone with more context; please don't leave this pending. 🙏

Questions?

Hop in the #help-i18n-and-translation Slack channel.

This reverts commit a22bf00, reversing changes made to 0c74d64.

This reverts commit 000c23b.

ludoboludo

👌

andrewetchen · 2022-07-27T01:13:02Z

Hey team,

The probot-CLA was recently deprecated and replaced with the shopify-cla action.

To successfully use the new shopify-cla status check, this PR will be closed and reopened.

Thanks and sorry for the minor inconvenience. Message me if you have any questions 😄

Please refer to the following for more information:

translation-platform · 2024-11-05T19:03:14Z

locales/en.default.schema.json

@@ -360,6 +360,7 @@
  "sections": {
    "all": {
      "padding": {
+        "block_padding_heading": "Block padding",


Localization quality issue found

The following issues may affect the quality of localized translations if they are not addressed:

The value Block padding for key sections.all.padding.block_padding_heading is very short. Short strings are more likely to be misunderstood by translators without context. Please provide additional context for the translators if possible.

Please look out for other instances of this issue in your PR and fix them as well if possible.

Questions about these messages? Hop in the #help-localization Slack channel.

Add padding controls to the announcement bar

b00831b

kjellr added the Category: Enhancement New feature or request label Jun 14, 2022

kjellr self-assigned this Jun 14, 2022

kjellr commented Jun 14, 2022

View reviewed changes

ludoboludo reviewed Jun 17, 2022

View reviewed changes

kjellr mentioned this pull request Jun 21, 2022

Add option to adjust announcement bar font size #1778

Open

7 tasks

kjellr added 2 commits June 30, 2022 14:43

Remove conflicting CSS rule.

25381d3

Introduce block-specific label strings.

f83ebdc

ludoboludo reviewed Jun 30, 2022

View reviewed changes

Use a more general translation string.

009eecf

This reverts commit f83ebdc.

ludoboludo previously approved these changes Jul 4, 2022

View reviewed changes

Update 20 translation files

0c74d64

translation-platform bot dismissed ludoboludo’s stale review via 0c74d64 July 7, 2022 19:02

kjellr added 4 commits July 7, 2022 16:09

Merge branch 'main' into try/announcement-bar-padding

a22bf00

Revert "Merge branch 'main' into try/announcement-bar-padding"

000c23b

This reverts commit a22bf00, reversing changes made to 0c74d64.

Revert "Revert "Merge branch 'main' into try/announcement-bar-padding""

7bdea60

This reverts commit 000c23b.

Correct merge conflict.

e722b8b

ludoboludo previously approved these changes Jul 11, 2022

View reviewed changes

andrewetchen closed this Jul 27, 2022

andrewetchen reopened this Jul 27, 2022

kjellr and others added 2 commits September 2, 2022 08:00

Merge branch 'main' into try/announcement-bar-padding

c717a22

Update 1 translation file

97da9d3

translation-platform bot dismissed ludoboludo’s stale review via 97da9d3 October 12, 2022 13:01

translation-platform bot added 2 commits October 20, 2022 19:00

Update 1 translation file

2c2cbd2

Update 1 translation file

b80b663

translation-platform bot reviewed Nov 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add padding controls to the announcement bar #1789

Add padding controls to the announcement bar #1789

kjellr commented Jun 14, 2022 •

edited

Loading

kjellr Jun 14, 2022

ludoboludo Jun 17, 2022

ludoboludo Jun 17, 2022

kjellr Jun 14, 2022

ludoboludo Jun 17, 2022

kjellr Jun 30, 2022 •

edited

Loading

ludoboludo Jun 30, 2022

ludoboludo Jun 17, 2022

ludoboludo Jun 17, 2022

ludoboludo Jun 17, 2022

ludoboludo left a comment •

edited

Loading

ludoboludo Jun 30, 2022

kjellr Jul 1, 2022

ludoboludo left a comment

translation-platform bot commented Jul 5, 2022 •

edited by kjellr

Loading

ludoboludo left a comment

andrewetchen commented Jul 27, 2022 •

edited

Loading

translation-platform bot Nov 5, 2024

Add padding controls to the announcement bar #1789

Are you sure you want to change the base?

Add padding controls to the announcement bar #1789

Conversation

kjellr commented Jun 14, 2022 • 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

kjellr Jun 30, 2022 • 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

ludoboludo left a comment • edited Loading

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

translation-platform bot commented Jul 5, 2022 • edited by kjellr Loading

Click the following links to answer the questions

Not your issue?

Questions?

ludoboludo left a comment

Choose a reason for hiding this comment

andrewetchen commented Jul 27, 2022 • edited Loading

translation-platform bot Nov 5, 2024

Choose a reason for hiding this comment

kjellr commented Jun 14, 2022 •

edited

Loading

kjellr Jun 30, 2022 •

edited

Loading

ludoboludo left a comment •

edited

Loading

translation-platform bot commented Jul 5, 2022 •

edited by kjellr

Loading

andrewetchen commented Jul 27, 2022 •

edited

Loading