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

Mimir 1849 endre punktum til desimaltegn tooltip engelske highchart #2097

Conversation

Carl-OW
Copy link
Contributor

@Carl-OW Carl-OW commented Sep 14, 2023

Fix for mimir-1849 og mimir-1851

changed decimalPoint from " , " to " . " in en version
changed download text from nb to en (still need fix for nb to nn)
Copy link
Contributor

@johnnadeluy johnnadeluy left a comment

Choose a reason for hiding this comment

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

I suggest only including the translated phrases in the highchart-lang-en file to avoid duplication, and to then merge the two into a new json object when defining the english phrases if all the other phrases are intended to be in norwegian for english pages.
Alternatively, if the default language for highcharts is english then we can consider dropping the highcharts-lang-en.json file and only set accessibility options when the chart has to be translated to norwegian. Or keep the highchart-lang-en file with only the necessary english overwrites/rephrasing.

Also, don't forget to apply the changes to the Highmap and nameSearch components as well.

@Carl-OW
Copy link
Contributor Author

Carl-OW commented Sep 19, 2023

I have deleted highmap-lang-en.json and opted for another solution, where the default highcharts language is used for for the en version of the site and the accessibilityLang file is used for nb version, I also added the fix for nameSearch, so that the decimalPoints for the graphs are correct for each language, as well as the download-dropdown text, (mimir-1849 og mimir-1851) - also tested that voice-over works correctly

Copy link
Contributor Author

@Carl-OW Carl-OW left a comment

Choose a reason for hiding this comment

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

updated code fix, and changed nameSearch.ts to return "nb", instead of "no"

Copy link
Contributor

@johnnadeluy johnnadeluy left a comment

Choose a reason for hiding this comment

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

Looks good! Good job 🌟 You could probably also get rid of the commented out code before merge 😊

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Carl-OW Carl-OW merged commit 96bb0d9 into master Sep 25, 2023
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