-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat: add translation module for labels #11
Conversation
WalkthroughThis pull request adds translation support to the osmosys-form component. It introduces the "@ngx-translate/core" dependency in package.json. The HTML template is updated to wrap user-facing texts (labels, placeholders, validation messages, and options) with translation function calls. In the TypeScript file, two new input properties (useTranslation and translateService) and a new translate(key: string) method are added to conditionally apply translations based on user configuration. Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
projects/osmosys-form/src/lib/osmosys-form.component.ts (1)
60-68
: Add TranslateService validation in ngOnInit.When translation is enabled, the component should validate that TranslateService is properly provided.
Add this validation to ngOnInit:
ngOnInit(): void { + if (this.useTranslation && !this.translateService) { + console.error('TranslateService is required when useTranslation is enabled'); + } + this.formConfig = this.formJsonData || {}; if (this.formConfig.layout?.rows) { this.createForm(this.formConfig.layout.rows); } this.formInstance.emit(this.form); }
🧹 Nitpick comments (3)
projects/osmosys-form/src/lib/osmosys-form.component.html (2)
39-39
: Consider security implications of autocomplete attribute.The autocomplete attribute has been added with a default of 'off' for all form elements. While this enhances security, it might affect user experience for non-sensitive fields.
Consider making the autocomplete value configurable through the form configuration:
-[autocomplete]="element.autocomplete || 'off'" +[autocomplete]="element.autocomplete || element.type === 'password' ? 'off' : 'on'"Also applies to: 75-75, 123-123, 163-163, 198-198, 233-233, 286-286, 327-327, 368-368, 409-409
39-39
: Enhance autocomplete attributes with semantic values.While setting autocomplete to 'off' by default is a good start, consider using semantic values for better user experience.
For example:
-[autocomplete]="element.autocomplete || 'off'" +[autocomplete]="element.autocomplete || getDefaultAutocomplete(element.type, element.name)"Add this helper method to the component:
private getDefaultAutocomplete(type: string, name: string): string { switch (type) { case 'input': if (name.includes('email')) return 'email'; if (name.includes('phone')) return 'tel'; if (name.includes('name')) return 'name'; return 'off'; case 'password': return 'new-password'; default: return 'off'; } }Also applies to: 75-75, 123-123, 163-163, 198-198, 233-233, 286-286, 327-327, 368-368, 409-409
projects/osmosys-form/src/lib/osmosys-form.component.ts (1)
188-200
: Add type safety and null checks to the translate method.While the implementation is functional, it could benefit from improved type safety and null handling.
Consider this enhanced implementation:
- translate(key: string): string { + translate(key: string | null | undefined): string { + if (!key) { + console.warn('Translation key is empty'); + return ''; + } + if (this.useTranslation && this.translateService) { - if (key) { - return this.translateService.instant(key) || key; - } - - // eslint-disable-next-line no-console - console.warn(`Translation key is empty: ${key}`); - return ''; // Return an empty string if the key is invalid + const translated = this.translateService.instant(key); + return translated || key; } return key; // Return the key as fallback if translation is not used }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.json
is excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.json
(1 hunks)projects/osmosys-form/src/lib/osmosys-form.component.html
(22 hunks)projects/osmosys-form/src/lib/osmosys-form.component.ts
(3 hunks)
🔇 Additional comments (4)
projects/osmosys-form/src/lib/osmosys-form.component.ts (3)
22-22
: LGTM! The translation service integration looks good.The implementation correctly imports TranslateService and adds input properties with appropriate defaults for backward compatibility.
Also applies to: 38-40
188-200
: Consider adding unit tests for the translate method.The translate method implementation looks good with proper null checks and fallback handling. However, it's missing unit tests to verify the behavior.
Would you like me to generate unit tests for the translate method to cover:
- Translation with valid key
- Translation with empty key
- Translation when service is not provided
- Translation when useTranslation is false
22-22
: LGTM! Translation service integration looks good.The TranslateService import and input properties are properly defined with good defaults for backward compatibility.
Also applies to: 38-40
package.json (1)
24-24
: Verify compatibility between Angular 18 and ngx-translate 15.The project uses Angular 18, but @ngx-translate/core is pinned to version 15. Let's verify if this version is compatible with Angular 18.
OsmosysForm PR Checklist
Pre-requisites
PR Details
PR details have been updated as per the given format (see below)
feat: add dynamic form validation
).Additional Information
ready for review
should be added if the PR is ready to be reviewed).Development Specific
ng test
) (optional).ng build osmosys-form
to ensure the library builds without errors.Description:
This PR introduces robust translation support to the osmosys-forms component, allowing seamless integration of multilingual capabilities within user applications. The implementation leverages the ngx-translate language service to dynamically translate field labels, placeholders, and validation error messages based on the user's selected language.
Related changes:
Screenshots:

Pending actions:
Add a list of any pending actions that have or would require to be done in this PR.
Usage guide:
Users must have translation support set up in their application where they are using the osmosys-forms component. This includes configuring the ngx-translate service and defining translation files with appropriate keys and values.
Users can enable translation by passing the useTranslation flag as true and providing the translation service to the osmosys-forms component.
Additional notes:
Related issues:
close #7
Summary by CodeRabbit