-
Notifications
You must be signed in to change notification settings - Fork 87
Internationalization #193
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
Internationalization #193
Conversation
Signed-off-by: Vedansh Saini <[email protected]>
Signed-off-by: Vedansh Saini <[email protected]>
Reviewer's GuideThis PR implements internationalization by annotating UI elements in popup.html with data-i18n attributes, introducing locale JSON files under src/_locales, and wiring up translation logic in popup.js using the chrome.i18n API, along with updating manifest.json to specify the default locale. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @vedansh-5 - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/scripts/popup.js:240` </location>
<code_context>
// Allow empty org to fetch all GitHub activities
chrome.storage.local.set({ orgName: org }, () => {
- generateBtn.innerHTML = '<i class="fa fa-spinner fa-spin"></i> Generating...';
+ generateBtn.innerHTML = `<i class="fa fa-spinner fa-spin"></i> ${chrome.i18n.getMessage('generatingButton')}`;
generateBtn.disabled = true;
window.generateScrumReport();
</code_context>
<issue_to_address>
No fallback for missing 'generatingButton' i18n key.
If the i18n key is missing, only the spinner appears. Please add a fallback string to improve user experience.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
generateBtn.innerHTML = `<i class="fa fa-spinner fa-spin"></i> ${chrome.i18n.getMessage('generatingButton')}`;
=======
const generatingText = chrome.i18n.getMessage('generatingButton') || 'Generating...';
generateBtn.innerHTML = `<i class="fa fa-spinner fa-spin"></i> ${generatingText}`;
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `src/scripts/popup.js:458` </location>
<code_context>
const scrumReport = document.getElementById('scrumReport');
if (scrumReport) {
- scrumReport.innerHTML = '<p style="text-align: center; color: #666; padding: 20px;">Organization changed. Click Generate button to fetch the GitHub activities.</p>';
+ scrumReport.innerHTML = `<p style="text-align: center; color: #666; padding: 20px;">${chrome.i18n.getMessage('orgChangedMessage')}</p>`;
}
// Clear the githubCache for previous org
</code_context>
<issue_to_address>
No fallback for missing 'orgChangedMessage' i18n key.
Without a fallback, the report may display nothing if the key is missing. Please add a default message to ensure the UI remains informative.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if (scrumReport) {
scrumReport.innerHTML = `<p style="text-align: center; color: #666; padding: 20px;">${chrome.i18n.getMessage('orgChangedMessage')}</p>`;
}
=======
if (scrumReport) {
const orgChangedMsg = chrome.i18n.getMessage('orgChangedMessage') || 'Organization changed. Click Generate button to fetch the GitHub activities.';
scrumReport.innerHTML = `<p style="text-align: center; color: #666; padding: 20px;">${orgChangedMsg}</p>`;
}
>>>>>>> REPLACE
</suggested_fix>
Help me be more useful! Please click π or π on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Vedansh Saini <[email protected]>
@vedansh-5 it doesn't work, please see screenshot |
That's really weird, because |
@Preeti9764 Could you also review this and see if the same error persists for you. Thanks |
Signed-off-by: Vedansh Saini <[email protected]>
@Preeti9764 Please take a look whenever you can, thanks, |
@Preeti9764 Please review this PR and share your views on this. Thanks! |
@vedansh-5 please solve conflicts and we don't need to translate the name of the extension, just like other names "GitHub or GitLab" |
Signed-off-by: Vedansh Saini <[email protected]>
Signed-off-by: Vedansh Saini <[email protected]>
Signed-off-by: Vedansh Saini <[email protected]>
Signed-off-by: Vedansh Saini <[email protected]>
@vedansh-5 how can a user use this feature? Thanks |
I switched my browser to Vn but I didn't see the translated version of the extension |
There have been some issues after a rebase, I'd resolve them soon. |
Signed-off-by: Vedansh Saini <[email protected]>
@hpdang @Preeti9764 Please test the new code changes. It works properly now. Thankyou! |
Signed-off-by: Vedansh Saini <[email protected]>
@Preeti9764 @hpdang Please review this whenever you have some free time, thankyou. |
It still doesn't work for me actually. Maybe I missed as a step? How should I test it? I set my browser preferred language to Vietnamese. And other app such as Eventyay UI is automatically translated to Vn. |
Missing a step would be unlikely, You have to change the preferred language and then relaunch the browser. This should automatically translate the extension. |
@hpdang |
@hpdang @vedansh-5 when i switch my broswer deafult language it translate for my extension language too, working for me.Thanks! |
π Fixes
Fixes #159
π Summary of Changes
All the labels of extension changes according to the default language set in users' browser.
πΈ Screenshots / Demo (if UI-related)
β Checklist
π Reviewer Notes
Add any special notes for the reviewer here
Summary by Sourcery
Externalize all user-facing text into localized message files and apply internationalization across the extensionβs UI based on the userβs browser language.
New Features:
Enhancements: