-
Notifications
You must be signed in to change notification settings - Fork 15
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
fix: translate validation text inside validation functions #1253
base: master
Are you sure you want to change the base?
Conversation
🚀 Deployed on https://pr-1253--dhis2-ui.netlify.app |
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.
@tomzemp do you think this could have a visible performance implication, having to call i18n.t several times rather than once? I am thinking for example if we're using the validators inside a table or a loop .. I am also noting that we are still using a version of i18next from 5 years ago so might have its issues anyhow.
Being a bit extra-conservative, could we perhaps force reinitialising i18next in the case of the Login app?
I don't think that this should be an issue, these are primitive functions with little computation, just grabbing values by key from an object. If you're very worried, I suppose the code could be changed so that the translation function is only called when the translation is needed: Before: const boolean = (value) => {
const invalidBooleanMessage = i18n.t('Please provide a boolean value')
return isEmpty(value) || typeof value === 'boolean'
? undefined
: invalidBooleanMessage
} After: const boolean = (value) => {
if (isEmpty(value) || typeof value === 'boolean') {
return undefined
}
return i18n.t('Please provide a boolean value')
} |
I don't think that this should block this PR from being merged as this seems to be an edge case. A simple fix would be to increment a |
it does a bit more than key lookup but none of it looks expensive to be fair, also none of it seems to have changed in the 5 years since we introduced that version of the library. I guess my point is that if re-initialising i18next solves the login app use case, then maybe we can just do that (when the user changes language in the login screen) rather than changing the UI components here. I am not sure if that's feasible though with the wrapper we have around i18next - in that case, this solution looks good. |
Ah yeah, fair enough. I didn't expect that much to be going on..
Yeah, worth a try I guess |
Thanks. My initial reaction was similar to @Mohammer5 's and I thought this would basically be a negligible key/value lookup, but I did a basic test in an app to try to separate out the additional time associated with the i18n.t functionality vs. just using a stored translation:
Sample run for this was 8ms elapsed without the translation and 3343ms elapsed with translation (1 million iterations) (I'm on a slow machine today, though). That is significantly worse performance wise; I'm not sure, however, if this would actually be noticeable (e.g. I think the rendering of a million error messages would cause a time out, so within the context of how these would be used, the performance hit should hopefully not be noticeable). That said, I think @Mohammer5's proposal of only accessing the translation if there is a violation would make sense and would also get rid of most of the performance hit (assuming that we generally expect things to be valid). With regards to the alternate approach of reinitializing i18next: I'm not sure I follow how this would work? I think the issue with these validation functions is that the translation is defined at the time of the module import (so if my language, were say Norwegian, when the module was imported, they will remain in Norwegian even if the i18next language changes until the module is re-imported, which I think will only occur when the page is refreshed?). That is, I'm not sure if we can do anything with i18next from the login app because the error message string is a constant in an already imported module? If we think it's not worth it to update the translation approach for the validators (because only the login app allows you to change language on the page), I could also just write a couple of custom validators for the login app to avoid this issue.
Yeah. Good point. We can just force the rerender 👍; I guess it's just an extra step, but not a big deal. |
Passing run #2895 ↗︎
Details:
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
FYI. I updated logic to move the translation until after the error was identified (as suggested by @Mohammer5). I will put this PR on hold, though, until we decide the overall approach for login app customization, as that may affect how we handle language selection. If we use web components for customization, the app may need to be refreshed when changing language, which would make this PR unnecessary. |
Implements LIBS-483
Description
Currently the translation strings in form validators are defined outside of the exported validator function. This means that the translation is a constant, generated when the module gets imported. This works generally as users have to change their selected language and then navigate to another app, but this does not work in the context of the login app where the language selection is on the page, and we do not want to have to force refresh in order for translations to flow through.
Known issues
Checklist
The changes here do not change any functionality, so just updating the existing tests to pass seems sufficient.
Screenshots