-
Notifications
You must be signed in to change notification settings - Fork 3
Localisation ekyc #523
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: localisation-biometric-kyc
Are you sure you want to change the base?
Localisation ekyc #523
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
| function applyPageTranslations() { | ||
| document.querySelectorAll('[data-i18n]').forEach((el) => { | ||
| const key = el.getAttribute('data-i18n'); | ||
| if (key) { | ||
| try { | ||
| el.textContent = translate(key); | ||
| } catch (e) { | ||
| console.error(`Translation failed for key: ${key}`, e); | ||
| } | ||
| } | ||
| }); | ||
| } |
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.
Suggestion: Using textContent will remove any child HTML elements (like icons or nested spans). This could break UI components that contain both text and HTML. Consider checking if the element has child nodes before replacing content, or use a data attribute to mark text-only elements. [possible issue, importance: 9]
| function applyPageTranslations() { | |
| document.querySelectorAll('[data-i18n]').forEach((el) => { | |
| const key = el.getAttribute('data-i18n'); | |
| if (key) { | |
| try { | |
| el.textContent = translate(key); | |
| } catch (e) { | |
| console.error(`Translation failed for key: ${key}`, e); | |
| } | |
| } | |
| }); | |
| } | |
| function applyPageTranslations() { | |
| document.querySelectorAll('[data-i18n]').forEach((el) => { | |
| const key = el.getAttribute('data-i18n'); | |
| if (key) { | |
| try { | |
| if (el.children.length === 0) { | |
| el.textContent = translate(key); | |
| } else { | |
| const textNodes = Array.from(el.childNodes).filter(node => node.nodeType === Node.TEXT_NODE); | |
| if (textNodes.length === 1) { | |
| textNodes[0].textContent = translate(key); | |
| } | |
| } | |
| } catch (e) { | |
| console.error(`Translation failed for key: ${key}`, e); | |
| } | |
| } | |
| }); | |
| } |
| await setCurrentLocale(config.translation?.language || 'en'); | ||
| document.documentElement.dir = getDirection(); | ||
| applyPageTranslations(); | ||
| document.querySelector('main').hidden = false; |
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.
Suggestion: If applyPageTranslations() fails or throws an error, the main element will remain hidden, leaving users with a blank screen. Wrap the translation logic in a try-catch block to ensure the UI is always shown, even if translations fail. [general, importance: 7]
| await setCurrentLocale(config.translation?.language || 'en'); | |
| document.documentElement.dir = getDirection(); | |
| applyPageTranslations(); | |
| document.querySelector('main').hidden = false; | |
| await setCurrentLocale(config.translation?.language || 'en'); | |
| document.documentElement.dir = getDirection(); | |
| try { | |
| applyPageTranslations(); | |
| } catch (e) { | |
| console.error('Failed to apply page translations', e); | |
| } | |
| document.querySelector('main').hidden = false; |
beaglebets
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.
I'm curious how we're going to handle the bank names themselves.
PR Type
Enhancement
Description
Add internationalization (i18n) support to eKYC form with translation keys
Implement dynamic page translations for form labels, placeholders, and messages
Add Arabic locale translations for ID information fields
Set document direction based on locale (RTL/LTR support)
File Walkthrough
ekyc.js
Implement translation utilities and dynamic page translation logicpackages/embed/src/js/ekyc.js
translate,translateHtml,getDirection)from localisation module
applyPageTranslations()function to dynamically translate elementswith
data-i18nattributestranslation keys
initialization
ekyc.html
Add translation attributes to HTML elements for i18n supportpackages/embed/src/ekyc.html
data-i18nattributes to all user-facing text elements (headings,labels, buttons)
messages
ar-EG.json
Add Arabic translations for ID information form fieldspackages/web-components/locales/ar-EG.json
bank,selectBank,searchBank,noBankFound)en-GB.json
Add English translations for ID information form fieldspackages/web-components/locales/en-GB.json
bank,selectBank,searchBank,noBankFound)