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

Dawn settings update #3683

Open
wants to merge 89 commits into
base: main
Choose a base branch
from
Open

Dawn settings update #3683

wants to merge 89 commits into from

Conversation

katycobb
Copy link

@katycobb katycobb commented Dec 10, 2024

PR Summary:

Our new dense UI makes the online store editor look and feel more like a pro tool, but the settings labels weren't built for this new interface. This is resulting in some too-long text which wraps and makes Dawn's settings look more crowded and tougher to scan than they should be.

This is also an opportunity to bring our settings content up to date based on our newest Polaris content uplift guidelines.

Why are these changes introduced?

Fixes #0.

What approach did you take?

Other considerations

Decision log

# Decision Alternatives Rationale Downsides
1

Visual impact on existing themes

Testing steps/scenarios

  • Step 1

Demo links

Checklist

Copy link
Contributor

@Roi-Arthur Roi-Arthur left a comment

Choose a reason for hiding this comment

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

Here's a pass purely on settings wording

locales/en.default.schema.json Outdated Show resolved Hide resolved
locales/en.default.schema.json Outdated Show resolved Hide resolved
locales/en.default.schema.json Outdated Show resolved Hide resolved
locales/en.default.schema.json Show resolved Hide resolved
locales/en.default.schema.json Outdated Show resolved Hide resolved
locales/en.default.schema.json Outdated Show resolved Hide resolved
locales/en.default.schema.json Show resolved Hide resolved
locales/en.default.schema.json Show resolved Hide resolved
locales/en.default.schema.json Outdated Show resolved Hide resolved
locales/en.default.schema.json Show resolved Hide resolved
@tyleralsbury tyleralsbury force-pushed the setting-content-audit branch from e657c7c to 969acea Compare January 30, 2025 17:04
@katycobb katycobb marked this pull request as ready for review February 1, 2025 00:13
"header__2": {
"content": "Announcements"
"heading_utilities": {
"content": "Utilities"
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 Utilities for key sections.announcement-bar.settings.heading_utilities.content 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
Author

Choose a reason for hiding this comment

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

Utilities is a heading for a group of settings like language selector and social media links that a merchant may want to toggle on to appear in their announcement bar.

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 comment is meant to be in the code actually. But it's okay. We can keep it as is for now I'd say.

Copy link
Author

Choose a reason for hiding this comment

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

Oop my bad! @tyleralsbury led me astray 😃. @ludoboludo if you're familiar with how to add translation notes to the code I'd love to pair on it if you have time.

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 might just be not doable at present for us as JSON doesn't support comments 🤔 Maybe something ill bring to those folks and see what they say is best 😄

"content": "Language selector",
"info": "To add a language, go to your [language settings.](/admin/settings/languages)"
"label": "Country/region selector",
"info": "[Manage countries/regions](/admin/settings/markets)"
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 [Manage countries/regions](/admin/settings/markets) for key sections.announcement-bar.settings.enable_country_selector.info 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
Author

Choose a reason for hiding this comment

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

Country/region selector is a toggle setting that allows merchants to decide whether they would like to display this feature on their online store. The info text links to the merchants' admin settings where they can manage which countries and markets they have configured.

"header__4": {
"content": "Language selector",
"info": "To add a language, go to your [language settings.](/admin/settings/languages)"
"label": "Country/region selector",
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 Country/region selector for key sections.announcement-bar.settings.enable_country_selector.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
Author

@katycobb katycobb Feb 3, 2025

Choose a reason for hiding this comment

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

Country/region selector is a toggle setting that allows merchants to decide whether they would like to display this feature on their online store.

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, just a few comments while we wait for the rest of the translations

Comment on lines -182 to +180
"info": "Selecting a different font can affect the speed of your store. [Learn more about system fonts.](https://help.shopify.com/manual/online-store/os/store-speed/improving-speed#fonts)"
"label": "Font"
Copy link
Contributor

Choose a reason for hiding this comment

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

Too bad we're losing the tidbits on performance/educating the users.

Copy link
Author

Choose a reason for hiding this comment

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

This info lives inside the font picker as well, which I think is actually the better placement since the merchant would have shown shown intent to change the font first 🙂

locales/en.default.schema.json Show resolved Hide resolved
"header__2": {
"content": "Announcements"
"heading_utilities": {
"content": "Utilities"
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 comment is meant to be in the code actually. But it's okay. We can keep it as is for now I'd say.

Comment on lines +611 to +613
"header_layout": {
"content": "Layout"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

We have many references to Layout across the file. There could be an opportunity in consolidating them like we do for sections.all.padding etc.
But it's not something that needs to happen now, it will mess with the translations we're getting back and add some more wait time.

Copy link
Author

Choose a reason for hiding this comment

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

Yes good call - it would make sense to unify it

locales/en.default.schema.json Show resolved Hide resolved
locales/en.default.schema.json Show resolved Hide resolved
@translation-platform
Copy link
Contributor

translation-platform bot commented Feb 5, 2025

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

Please answer the following questions.

Warning

Replies in GitHub are not visible to translators. Use the provided "Answer here" links. 🙅‍♀️

Note

Not your issue? Forward it to someone with more context; please don't leave it pending. 🙏
Questions? Hop in the #help-localization Slack channel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants