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

[Announcement bar] Change auto rotate announcements copy and min value #2787

Merged
merged 5 commits into from
Jul 8, 2023

Conversation

ludoboludo
Copy link
Contributor

PR Summary:

Why are these changes introduced?

Fixes #2785

What approach did you take?

Change the copy of the text and updated the min value. Though I wonder if we should change the step. Slideshow has 2 for step while announcements have 1. But that'd potentially break folks current setup if they used 6s and can now only have 5s or 7s though it doesn't super impactful.

Other considerations

Decision log

Visual impact on existing themes

None, should only be a new setting option (3s) and content change.

Testing steps/scenarios

  • Check the announcement bar and update the timing to 3s make sure it still works 🤷

Demo links

Checklist

@ludoboludo ludoboludo self-assigned this Jul 6, 2023
@katycobb katycobb mentioned this pull request Jul 6, 2023
3 tasks
},
"change_slides_speed": {
"label": "Change slides every"
"label": "Change every"
Copy link
Contributor

Choose a reason for hiding this comment

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

Localization quality issue found

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

  • The value Change every for key sections.announcement-bar.settings.change_slides_speed.label 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the screenshot to share more context around that change 👍

Copy link
Contributor

@lougoncharenko lougoncharenko left a comment

Choose a reason for hiding this comment

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

This looks good to me! I tested the following things and didn't find a way to break anything

  • Test the automatic slide on desktop version
  • Tested on mobile to ensure that the announcement bar doesn't rotate.
  • Added multiple announcement bars to ensure nothing broke
  • Test in safari and chrome
  • Applied color scheme to announcement bar to ensure nothing was broken
  • Checked and unchecked the Auto-rotate on desktop
  • tested with the new 3s min.
  • Tested with varying speeds
  • Added social icons to the announcement bar and tested with different speeds

I tried a couple different scenarios and wasn't able to find a way to break this! The code looks good to me and I am liking the label changes! They are user friendly!

@ludoboludo ludoboludo merged commit 594b5e0 into main Jul 8, 2023
5 checks passed
@ludoboludo ludoboludo deleted the auto-rotate-setting-text branch July 8, 2023 14:46
@kjellr kjellr mentioned this pull request Jul 10, 2023
8 tasks
phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024
Shopify#2787)

* Change auto rotate announcements copy and min value

* adjust copy based on feedback

* Update 4 translation files

* Update 14 translation files

* Update 2 translation files

---------

Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>
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.

[Announcement Bar] Improvements
5 participants