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

fix: standardize i18n string references #1119

Conversation

aasimsyed
Copy link
Contributor

@aasimsyed aasimsyed commented Feb 5, 2025

Issue: #1100

changes suggested by @andrewtavis

  • Replace hardcoded i18n path strings with i18nMap references in components
  • Update button labels and aria-labels to use i18nMap paths
  • Remove unnecessary i18nMap imports where direct strings are preferred
  • Maintain consistent i18n usage pattern across components

Contributor checklist


Description

Related issue

- Replace hardcoded i18n path strings with i18nMap references in components
- Update button labels and aria-labels to use i18nMap paths
- Remove unnecessary i18nMap imports where direct strings are preferred
- Maintain consistent i18n usage pattern across components
Copy link

netlify bot commented Feb 5, 2025

Deploy Preview for activist-org ready!

Name Link
🔨 Latest commit ba5793a
🔍 Latest deploy log https://app.netlify.com/sites/activist-org/deploys/67a50aa1e3812f00071499d1
😎 Deploy Preview https://deploy-preview-1119--activist-org.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

github-actions bot commented Feb 5, 2025

Thank you for the pull request! ❤️

The activist team will do our best to address your contribution as soon as we can. If you're not already a member of our public Matrix community, please consider joining! We'd suggest using Element as your Matrix client, and definitely join the General and Development rooms once you're in. Also consider attending our bi-weekly Saturday developer syncs! It'd be great to meet you 😊

Copy link
Contributor

github-actions bot commented Feb 5, 2025

Maintainer Checklist

The following is a checklist for maintainers to make sure this process goes as well as possible. Feel free to address the points below yourself in further commits if you realize that actions are needed :)

  • The TypeScript and formatting workflows within the PR checks do not indicate new errors in the files changed

  • The Playwright end to end and Zap penetration tests have been ran and are passing (if necessary)

  • The changelog has been updated with a description of the changes for the upcoming release and the corresponding issue (if necessary)

@andrewtavis
Copy link
Member

Quick note that we'll need many of these component attributes to be v-bind: equivalents, where rather than label=... we'd have :label=... which then interprets the value in the string as a variable rather than a string, @aasimsyed :) In case you haven't installed the Vue plugin for VS Code, I'd do that as that'll then show you explicitly that it's reading in a variable instead of a string :) Sorry if you do have it installed, just making sure as there have been some changes 😊 You can check the recommended extensions in the dropdowns in the Environment Setup section of the readme :)

@andrewtavis
Copy link
Member

And would be amazing if you could get the merge conflicts as well, but let me know if you need some help with those! :)

@andrewtavis
Copy link
Member

I spoke too soon :) Thanks, @aasimsyed! 🚀

@aasimsyed
Copy link
Contributor Author

Quick note that we'll need many of these component attributes to be v-bind: equivalents, where rather than label=... we'd have :label=... which then interprets the value in the string as a variable rather than a string, @aasimsyed :) In case you haven't installed the Vue plugin for VS Code, I'd do that as that'll then show you explicitly that it's reading in a variable instead of a string :) Sorry if you do have it installed, just making sure as there have been some changes 😊 You can check the recommended extensions in the dropdowns in the Environment Setup section of the readme :)

I'm not sure I understand what you mean here. You want those changes into this PR?

@andrewtavis
Copy link
Member

andrewtavis commented Feb 6, 2025

Yes these changes would need to happen on each component attribute where we're trying to pass the result of the object as a variable. As it's written now in some cases then the string "i18nMap..." will be passed. With the colon at the start the string is interpreted as a variable. We still leave the quotes on either side so it looks the same on the right side, but the colon on the left changes what's interpreted.

Let me know if it's still confusing!

- Migrate all static i18n key references to use v-bind syntax (:label/:ariaLabel)
@aasimsyed
Copy link
Contributor Author

Yes these changes would need to happen on each component attribute where we're trying to pass the result of the object as a variable. As it's written now in some cases then the string "i18nMap..." will be passed. With the colon at the start the string is interpreted as a variable. We still leave the quotes on either side so it looks the same on the right side, but the colon on the left changes what's interpreted.

Let me know if it's still confusing!

@andrewtavis please review. I think I did what you were asking for. Let me know if there are any other issues.

@andrewtavis andrewtavis self-requested a review February 6, 2025 17:52
Copy link
Member

@andrewtavis andrewtavis left a comment

Choose a reason for hiding this comment

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

A couple more instances needed to get changed, @aasimsyed, and with that I did a few updates for things that weren't quite working after some dependency updates.

Thanks so much for bringing this home, @aasimsyed! Really awesome to have this setup finalized 🚀

CC @OmarAI2003 that the i18nMap object is now in full use over here. One extra thing I realized for i18n-check is that we need to give the user the ability for the map file to be written with a license header. I made the changes to add the current way we do it over here into the project, and then we'll just need to add an option for what the license for the project is :)

@andrewtavis
Copy link
Member

PS I ran the e2e tests and we're at the ~7 that are already failing, so we're good in those regards as well :)

@andrewtavis andrewtavis merged commit b0d8665 into activist-org:main Feb 6, 2025
8 checks passed
@andrewtavis andrewtavis mentioned this pull request Feb 6, 2025
2 tasks
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.

2 participants