hypertrons / hypertrons-crx

A browser extension for insights into GitHub projects and developers.
https://crx.hypertrons.io
Apache License 2.0
329 stars 91 forks source link

【Task 1】Refactor the existing multiple languages feature by adopting a third i18n library #826

Closed taketaketakeru closed 2 hours ago

taketaketakeru commented 3 weeks ago

Brief Information

This pull request is in the type of (more info about types):

Related issues (Task1 i18n):

Details

Refactor the existing multiple languages feature by adopting a third i18n library, and fix the bug #804 . If there are still issues, please let me know.

Checklist

CLAassistant commented 3 weeks ago

CLA assistant check
All committers have signed the CLA.

wxharry commented 3 weeks ago

Hi, sorry for the delay reply. I have gone through your work quickly and looks like everything is on the right track. If you still have further question on what we are doing for this chagne, I will reply in the related issue. For your current work, (I am not giving reviews, just trying to make your future work easier), we might want to change our message files, maybe change its name to translation.json and simplify its structure as i18next recommended:

{
  "welcome": "Welcome to React and react-i18next",
  "change_language": "Change Language"
}

For sure there will be a lot of changes but feel free to make changes if it is necessary. :) cc @tyn1998

taketaketakeru commented 3 weeks ago

Hi, sorry for the delay reply. I have gone through your work quickly and looks like everything is on the right track. About your questions, I will reply in the related issue in details. For your current work, (I am not giving reviews, just trying to make your future work easier), we might want to change our message files, maybe change its name to translation.json and simplify its structure as i18next recommended:

{
  "welcome": "Welcome to React and react-i18next",
  "change_language": "Change Language"
}

For sure there will be a lot of changes but feel free to make changes if it is necessary. :) cc @tyn1998

Thank you very much for your suggestions! They've been really helpful. I'll keep refining the code. 😄

wxharry commented 2 weeks ago

Hi, thanks for the contributions. Please feel free to remove code and delete files when working on it. Git keeps the history for us, we can recover anything deleted in the branch. So, if you think such code is no longer needed, delete it instead of commenting it out.

taketaketakeru commented 1 week ago

Hi, thanks for the contributions. Please feel free to remove code and delete files when working on it. Git keeps the history for us, we can recover anything deleted in the branch. So, if you think such code is no longer needed, delete it instead of commenting it out.

OK ;-)

wxharry commented 1 week ago

Looks like you are having a conflict in this PR, you can look into git merge or git rebase command to resolve it. Let me know if you have trouble resolving it.

BTW, Don't forget to sign the Contributor License Agreemenet

taketaketakeru commented 1 week ago

Looks like you are having a conflict in this PR, you can look into git merge or git rebase command to resolve it. Let me know if you have trouble resolving it.

BTW, Don't forget to sign the Contributor License Agreemenet

Sorry for the mistakes! :-O
I was confused before until I found I signed the Contributor License Agreement with an unmatched email address. Then, I resolved the conflict issues and the CLA issues. Now, it seems to look good. Please let me know if there is still something wrong. :-D

wxharry commented 1 week ago

Sorry for the mistakes! :-O

This is not your fault. It is common to have conflicts since others are also working on it.

Is this PR ready to review?

taketaketakeru commented 1 week ago

This is not your fault. It is common to have conflicts since others are also working on it.

Is this PR ready to review?

Yes, it's ready for review. :-D

taketaketakeru commented 1 week ago

Nice work! I have left a few comments that will improve your work and understand more about the library. lmk if you have any questions.

Is #804 fixed in this PR? I am still having the issue. (Just asking, this is not mandatory for this branch)

I refined the code according to the review comments. :-D

As for #804, since I added the i18n.changeLanguage(options.locale); in repo-header-labels\view.tsx as follows,

useEffect(() => {
    (async function () {
      setOptions(await optionsStorage.getAll());
      i18n.changeLanguage(options.locale);
    })();
  }, [options.locale]);

now it can display the corresponding language correctly under different language settings. However, this cannot take effect as soon as changing the language settings. It will only display the correct language after refreshing the webpage, unlike the star-button and the fork-button.

I think the reason for this is the repo-head-labels only be loaded once when initing the webpage, so the change of language setting will not take effect immediately since the repo-head-labels won't be reloaded. Even if I change the code as follows (add the options.locale in ), it won't help for the same reason. (I’m not pretty sure about the reason).

useEffect(() => {
    const placeholderElement = $('<div class="NativePopover" />').appendTo('body')[0];
    render(
      <>
        <NativePopover anchor={$('#activity-header-label')} width={280} arrowPosition="top-middle">
              ......
        </NativePopover>
      </>,
      placeholderElement
    );
  }, [**options.locale**]);

Since I'm not quite familiar with React, I don't know the exact reason for this and where to change to fix this problem. If you give me a more specific reason for the bug, I would like to try to fix the issue! 🧐 Or is the current solution as adding i18n.changeLanguage(options.locale); in repo-header-labels\view.tsx enough to fix this?

wxharry commented 1 week ago

now it can display the corresponding language correctly under different language settings. However, this cannot take effect as soon as changing the language settings. It will only display the correct language after refreshing the webpage, unlike the star-button and the fork-button. I think the reason for this is the repo-head-labels only be loaded once when initing the webpage, so the change of language setting will not take effect immediately since the repo-head-labels won't be reloaded. Even if I change the code as follows (add the options.locale in ), it won't help for the same reason. (I’m not pretty sure about the reason). Since I'm not quite familiar with React, I don't know the exact reason for this and where to change to fix this problem. If you give me a more specific reason for the bug, I would like to try to fix the issue! 🧐 Or is the current solution as adding i18n.changeLanguage(options.locale); in repo-header-labels\view.tsx enough to fix this?

That is highly likely. Then I guess we can fix the issue essentially by extracting the tooltip chart out of the labels and injecting it to the labels same as other charts, so the labels and charts can have different injecting mechanism. This is not our focus in this pr, but could you look into it a bit deeper to see if your theory is correct? We can update anything related to that issue.(Of course, if you can fix it, that would be great! 😄 )

taketaketakeru commented 6 days ago

That is highly likely. Then I guess we can fix the issue essentially by extracting the tooltip chart out of the labels and injecting it to the labels same as other charts, so the labels and charts can have different injecting mechanism. This is not our focus in this pr, but could you look into it a bit deeper to see if your theory is correct? We can update anything related to that issue.(Of course, if you can fix it, that would be great! 😄 )

Thank you for your advice! It feels like I know how to fix this bug! 😉 I will try and see if I can fix it when I have time! If there is any progress, I will comment here. 🧐

taketaketakeru commented 6 days ago

That is highly likely. Then I guess we can fix the issue essentially by extracting the tooltip chart out of the labels and injecting it to the labels same as other charts, so the labels and charts can have different injecting mechanism. This is not our focus in this pr, but could you look into it a bit deeper to see if your theory is correct? We can update anything related to that issue.(Of course, if you can fix it, that would be great! 😄 )

It seems I fixed the #804 issue, even though I'm not sure if it is the simplest way. 🧐 Please look into it and let me know if there is any problem! 😄

wxharry commented 2 hours ago

Nice work! Thanks for your contributions! appreciate it.