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

Standardize i18n keys #902

Closed
2 tasks done
andrewtavis opened this issue Jun 15, 2024 · 16 comments
Closed
2 tasks done

Standardize i18n keys #902

andrewtavis opened this issue Jun 15, 2024 · 16 comments
Assignees
Labels
-priority- High priority bug Something isn't working

Comments

@andrewtavis
Copy link
Member

Terms

Behavior

Something that came up recently is that the i18n keys are not standardized - i.e. sometimes we have kebab case or snake case. This issue is to check the keys and change them all to either snake case or camel case, with the reason for this being that we want to be able to easily replace full sections of the key.

@andrewtavis andrewtavis added the bug Something isn't working label Jun 15, 2024
@andrewtavis andrewtavis self-assigned this Jun 15, 2024
@andrewtavis andrewtavis added the -priority- High priority label Jun 15, 2024
@jannesblobel
Copy link

jannesblobel commented Jun 17, 2024

@andrewtavis I came upon your issue by chance.
What we are currently discussing internally in our team might also be of interest to you. Simply use human readable and random ids and let the tooling generate them. This will solve many problems

opral/monorepo#1892

aasimsyed pushed a commit to aasimsyed/activist that referenced this issue Jul 5, 2024
…r further work

<!---
Thank you for your pull request! 🚀
-->

### Contributor checklist

<!-- Please replace the empty checkboxes [] below with checked ones [x] accordingly. -->

- [x] This pull request is on a [separate branch](https://docs.github.com/en/get-started/quickstart/github-flow) and not the main branch

---

### Description

<!--
Describe briefly what your pull request proposes to change. Especially if you have more than one commit, it is helpful to give a summary of what your contribution is trying to solve.

Also, please describe shortly how you tested that your change actually works.
-->
- added missing `#` in `"#btn-become-supporter"`
- updated test on Landing Page to check this button (was missing)
- ran Playwright test to confirm passed

### Related issue

<!--- activist prefers that pull requests be related to already open issues. -->
<!--- If applicable, please link to the issue by replacing ISSUE_NUMBER with the appropriate number below. -->
<!--- Feel free to delete this section if this does not apply. -->

- #ISSUE_NUMBER
@jennethydyrova
Copy link
Contributor

hi @andrewtavis! I would like to help with this issue, is it still available?

@andrewtavis
Copy link
Member Author

Hey @jennethydyrova 👋 If you wanted to send along a PR that converts all of the kebab case to snake case, it'd be much appreciated!

@andrewtavis
Copy link
Member Author

Let me know if there's anything I can do to help :) I'll also send along a commit to this that will make a check to see if the keys can be effectively combined. We did discuss this a bit internally, and feel that the current system is valid as it allows the localization team to easily filter for parts of the app 😊

@jennethydyrova
Copy link
Contributor

I just wondered if would it be fine if I send all changes in one PR or would it be easier for you to review if I split it to multiple PRs?

@andrewtavis
Copy link
Member Author

I guess one PR would be easier as that would allow me to plan around it, bring in some others, and then merge it all at once and not need to deal with conflicts soon after that 😊

Thanks so much for asking!

@jennethydyrova
Copy link
Contributor

I've got one more question. According to style guide folder names should be separated by dash

Localization keys should be defined based on their component or page within the platform and the content that they refer to (CONTENT_REFERENCE below). Please use the following rules as a guide if you find yourself needing to create new localization keys:
Separate directories and references by . and CamelCase file name words by - in keys
Ex: "components.search-bar.CONTENT_REFERENCE" for the SearchBar component

then, for example, components.btn-action.save-settings key should be renamed to components.btn-action.save_settings, is that right?

@andrewtavis
Copy link
Member Author

Glad to have you working on this, @jennethydyrova! Looks like the styleguide needs to be updated as well. Let's use underscores for the whole thing. That was a poor choice by us earlier :)

Would you be able to send along an update to the styleguide with this as well?

@jennethydyrova
Copy link
Contributor

yes, sure! 🙂

@andrewtavis
Copy link
Member Author

Thank you, @jennethydyrova!

@andrewtavis
Copy link
Member Author

Closed by #948 🚀 Lots of work to get through this, @jennethydyrova, but with the new workflow we won't have to worry about naming keys anymore except for an identifier at the end for a hint for context. The prior parts of the key are based on the files in which it's used, and the new workflows will suggest corrections for those as we go 😊

Thanks so much for the contribution!

@jennethydyrova
Copy link
Contributor

Closed by #948 🚀 Lots of work to get through this, @jennethydyrova, but with the new workflow we won't have to worry about naming keys anymore except for an identifier at the end for a hint for context. The prior parts of the key are based on the files in which it's used, and the new workflows will suggest corrections for those as we go 😊

Thanks so much for the contribution!

Thank you, Andrew!

While addressing this issue, I noticed that the components.meta-tag-video.view-video key is utilized in frontend/components/card/search-result/CardSearchResult.vue, but it doesn’t seem to be defined in frontend/i18n/en-US.json. This might be something worth looking into.

@andrewtavis
Copy link
Member Author

Good catch, @jennethydyrova :) That's likely just a placeholder, but definitely should be removed at some point. Do you want to do a quick PR to fix it?

@jennethydyrova
Copy link
Contributor

Good catch, @jennethydyrova :) That's likely just a placeholder, but definitely should be removed at some point. Do you want to do a quick PR to fix it?

yes, sure! do you want this tag to be removed from labels?

@andrewtavis
Copy link
Member Author

Hmmm, good question :) Let's actually add it in. So this key's only in CardSearchResult, so based on the naming conventions we should change the instances of components.meta-tag-video.view-video to components.card_search_result.view_video, and then we can add this key and View video as it's value to the en-US i18n keys? How does this sound? You can link the PR to this issue, btw 😊

@jennethydyrova
Copy link
Contributor

hi, @andrewtavis! all done #966

andrewtavis added a commit that referenced this issue Sep 3, 2024
…ssing-key

Rename and add missing key to the keys list
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-priority- High priority bug Something isn't working
Projects
Archived in project
Development

No branches or pull requests

3 participants