-
-
Notifications
You must be signed in to change notification settings - Fork 4k
chore: update French i18n #5561
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
base: master
Are you sure you want to change the base?
Conversation
…in overlay and left panel with fallback options
✅ Deploy Preview for ohif-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
…ools, and Window Level Action Menu
…d new French terms
…terms in English and French
…u components in English and French
…ation integration
…ys for better localization support
…ls and add translations for English and French
…tion labels in ViewportDataOverlayMenu
…els and actions across multiple components
…pport for various labels and tooltips
…bar buttons, enhancing localization for user interface elements
…, enhancing localization in onboardingCustomization
… for width and height labels, improving localization in English and French
…r improved localization in English and French
…improved localization in English and French
…alization in English and French
|
Ready for review. I would like to use this to add another language next week. |
Merge branch 'master' of https://github.com/mbellehumeur/Viewers into chore/french
|
There is a Test-LNG folder that is used for testing and we try to keep it up-to-date. Could you please update that with ALL the strings you added? Grazie. |
Or I should say merci. 😊 |
jbocce
left a comment
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.
Très bien, monsieur. Merci beaucoup.
Thanks for this tremendous effort. A few comments to address.
I did not review the language files themselves, so if you put any profanity in there it is on you. 🤣🤣🤣
| segments: Segment[]; | ||
| placeholder?: string; | ||
| }) { | ||
| const { t } = useTranslation('SegmentationTable'); |
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.
I think a better name is SegmentationPanel since the table is technically part of the panel and it is best to keep ALL the segmentation strings in one place.
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.
All of the strings in here pertain to the segmentation panel, yet it looks like the strings were put into the buttons i18n file. Please reconsider this.
| */ | ||
| export function formatDICOMDate(date, strFormat = 'MMM D, YYYY') { | ||
| return moment(date, 'YYYYMMDD').format(strFormat); | ||
| export function formatDICOMDate(date, strFormat?: string) { |
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.
We have this function a few times. Can we put it someplace central, like maybe in ui-next/utils or something?
| let translatedEmpty = '(empty)'; | ||
| try { | ||
| // eslint-disable-next-line @typescript-eslint/no-var-requires | ||
| const i18n = require('@ohif/i18n').default; |
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.
I think using require here might be a no-no. @sedghi thoughts about require?
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.
similar to a comment I made about segmentation, these strings in this file are particular to TMTV, not sure if they should be lumped in with "buttons".
| }, | ||
| ref | ||
| ) => { | ||
| const { t } = useTranslation('MeasurementTable'); |
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.
I'm not sure it is the correct assumption to use MeasurementTable here because DataRow is supposed to be a generic ui component. It is used in various places including the segmentation panel. I think here we either
- create a DataRow i18n fine OR
- consider passing in the t as a property OR
- perhaps even the translation file name as a property OR
- put the strings into Common.js
I think I like the DataRow (first option best).
| maxWidth?: string; | ||
| maxHeight?: string; | ||
| /** Optional labels/placeholders for localization */ | ||
| widthLabel?: string; |
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.
From the file below it appears that there is an assumption here that these are already translated or am I missing something?


Context
Update french version with toolbar and other items.
Changes & Results
Add local date overlays and left panel.
Added translation for user preferences (hotkeys).
Fixed bug with the display of the hotkeys on first load.
Testing
Check menus and messages for non translated labels.
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment