fix: take into account user lang in Time component #1793
Conversation
Co-authored-by: tommoor <tom.moor@gmail.com>
|
|
app/components/Time.js
Outdated
| function getLocaleForUser(user) { | ||
| if (!user) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const userLanguage = user.language.split("_")[0]; | ||
| return require(`date-fns/locale/${userLanguage}`); | ||
| } |
There was a problem hiding this comment.
I moved the logic to a method outside the component, I wasn't sure if you prefer it on top of the comment or at the bottom, or even if you just prefer to have that logic inside the component, so any feedback is welcome.
How about we pull this out to hooks/useUserLocale ?
|
Tests are failing because of the snapshots related to the users api and user presenter tests, if we hardcode |
|
Sounds good 👌 |
|
Functionally this is good, but looking at the bundle size audit the require isn't working how we might have expected – all of the translations are getting included in an existing chunk because the require isn't async, causing a 3.5% increase to bundle size. In order to have the translations dynamically load we probably need to make these changes unless you have another idea:
Does this make sense? |
|
Yes, it makes a lot of sense, what I'm not sure about is what to move exactly to the I've never used Suspense before, so let me get this right, should we keep what we had before as the fallback, and create a new component which also has the tooltip and the time? Example: <React.Suspense fallback={
<Tooltip ... >
<time ... />
</Tooltip>
}>
<LocaleTime />
</React.Suspense> |
I'd be happy if the fallback was a dumb html |
|
Hmm, I'm not sure what I've done wrong, but it still shows Update: It looks like it still adds all the locales... maybe it's related to the dynamic require? |
|
Yeah that looks better, 0.49%, I'm not sure if there is a better way to load them, I don't really like loading them manually, it's pretty easy to add a new language to Another thing I don't like either is how it flahes from the fallback to the LocaleTime, but I'm not sure what else we can do... Let me know what you think! |
Co-authored-by: Tom Moor <tom.moor@gmail.com>
|
It took a while but we got there! 🚀 Let's hope it doesn't come back to bite us, locales are never easy 😅 |
|
Absolutely, thanks for your help |
Closes #1728
This PR takes into account the user selected language to format the time in the
Timecomponent. It uses the changes done in the functional-time branch.Also, I've added Tom as a Co-Author, since he was the one who transformed it into a functional component.
I moved the logic to a method outside the component, I wasn't sure if you prefer it on top of the comment or at the bottom, or even if you just prefer to have that logic inside the component, so any feedback is welcome.
I haven't tested it in a public page, since I'm not sure how to do it, how do I find a public page to test it? I've tried by publishing and sharing a document but I didn't see any dates.