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 Translation Method #1354

Merged
merged 6 commits into from
Oct 28, 2024

Conversation

lawsj
Copy link

@lawsj lawsj commented Oct 4, 2024

Description

Made changes to use the new "useTranslate()" instead of the old "translate()".

Partly Addresses #1204

Type of change

(Check the ones that apply by placing an "x" instead of the space in the [ ] so it becomes [x])

  • Note merging this changes the database configuration.
  • This change requires a documentation update

Checklist

(Note what you have done by placing an "x" instead of the space in the [ ] so it becomes [x]. It is hoped you do all of them.)

  • I have followed the OED pull request ideas
  • I have removed text in ( ) from the issue request
  • You acknowledge that every person contributing to this work has signed the OED Contributing License Agreement and each author is listed in the Description section.

##Limitations

N/a

@Rakesh-Ranga-Buram
Copy link
Contributor

Everything looks good to me. Please convert the draft to PR.

@Rakesh-Ranga-Buram
Copy link
Contributor

@huss , this PR only include changes related to translate() method and not formatMessage() method. Since formatMessage() changes are already been done on #1314

Copy link
Member

@huss huss left a comment

Choose a reason for hiding this comment

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

Thanks to @lawsj for another contribution. Overall, this works as desired. I made one comment in a file to address. In addition, here are comments that could not be placed in a file:

  • src/client/app/utils/translate.ts has the previously used translation function. I could not find it used (which is good). If so, I think this file should be removed to avoid unused code and to stop someone from using the old translate method.
  • OED is crashing when I go to a number of admin pages. For example, going to the conversion page and then try to create a conversion does this. What is strange is I cannot see why any change made in this PR would cause this to happen. Thus, I removed the new translate change in this file but it still happened. Next I tried changing to the development branch and it no longer crashed. This seems to imply it was a change in the pull request but why is unclear. Modest debugging did not find the reason. Given this, I wanted to know if you (or anyone else) sees this same type of failure? I want to know if it is a general problem or something in my setup.

src/client/app/components/BarControlsComponent.tsx Outdated Show resolved Hide resolved
@huss
Copy link
Member

huss commented Oct 17, 2024

This is just for the record: We are continuing to try to determine the cause of the issue. It may well be something outside the direct changes made in this PR.

@Rakesh-Ranga-Buram
Copy link
Contributor

@huss , the code has been updated with the discussed changes. Can you please review this PR

While two functions where in the components directory, they were
not in the function component so the React Hooks version of
translate should not be used. Before the change it was getting errors
and crashing. now it seems fine.
@huss
Copy link
Member

huss commented Oct 28, 2024

I pushed commits to do the following:

  1. Merge in development and address conflicts.
  2. Added missing CSV menu translations. Not directly related to this PR but similar so did it.
  3. Removed a couple of uses of useTranslate from maps because outside function component and causing issues.

Copy link
Member

@huss huss left a comment

Choose a reason for hiding this comment

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

Thanks to @Rakesh-Ranga-Buram for the updated code. Review and testing found it works as desired at this time. Congratulations to @lawsj on their first accepted contribution to OED.

@huss huss merged commit bef5173 into OpenEnergyDashboard:development Oct 28, 2024
3 checks passed
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.

3 participants