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

Update to conform to Material Design's style guide #4209

Open
wants to merge 1 commit into
base: 14-dev
Choose a base branch
from

Conversation

Susexe
Copy link

@Susexe Susexe commented Apr 4, 2024

Description

I updated the strings.xml file to conform to Material Design's style guide. Here's what's changed:

The first change is probably the most controversial one, but I would like to know your stance on this. Personally, sentence case looks much better and more natural to me on mobile devices. See the diff for full changes.

Type of change

✅ General change (non-breaking change that doesn't fit the below categories like copyediting)
❌ Bug fix (non-breaking change which fixes an issue)
❌ New feature (non-breaking change which adds functionality)
❌ Breaking change (fix or feature that would cause existing functionality to not work as expected)

@SuperDragonXD
Copy link
Collaborator

Hey there,

Regarding Title Case, this was once implemented by two of our former team members due to its “visual prominence” and “a more symmetrical appearance”. It is also implemented with inspiration from Apple’s UX style guide, and so that’s why it stuck here.

If we were to change this, I’d first ask the team internally regarding their stance on this change. Afterwards, I'll update you regarding this.

(Personally, I also like Title Case for the labels/headings. I prefer sentence cade for the rest of the UI though.)

Otherwise, the PR looks good. It seems like I forgot to update some other descriptions to match M3 guidelines, so thanks for the catch.

@Susexe
Copy link
Author

Susexe commented Apr 4, 2024

Thanks for the comment. Do let me know of your decision. I missed the following changes:

  • workspace_increase_max_grid_size_description is missing an article the,
  • show_suggested_apps_at_drawer_top is missing app in app drawer,
  • max_suggestion_result_count_title is missing web in web suggestions.

While we're at it, could you add me as a proofreader for Serbian (Cyrillic) and Serbian (Latin) on Crowdin? I've been active there for a long time, and I am willing to maintain the translations. Also, there are some approved strings that need to be modified. My username is Rancher. Thanks.

@SuperDragonXD
Copy link
Collaborator

SuperDragonXD commented Apr 4, 2024

For the latter, sure (it's been some time since I've touched Crowdin though, so it may take some time) (i seem to lack permissions, requesting it as of now). I'll look into the former later.

Copy link
Collaborator

@SuperDragonXD SuperDragonXD left a comment

Choose a reason for hiding this comment

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

After discussion with the team, it seems like they’re fine with the changes. However, regarding the three (3) strings above, I’d recommend keeping the original text for clarity. (As of now, haven’t gotten around to make all the strings more consistent with their name attribute)

Also regarding Proofreader permissions in Crowdin, I’ve added you there.

@Susexe
Copy link
Author

Susexe commented Apr 19, 2024

Sorry for misusing this thread, but could you also add Bosnian and Croatian to Crowdin? I can work on them too (they are all part of the macrolanguage known as Serbo-Croatian).

By the way, I fully completed the Serbian translation.

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.

None yet

2 participants