-
Notifications
You must be signed in to change notification settings - Fork 152
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(i18n): globally mocks translation function #11238
Conversation
25a4e2e
to
cc79fec
Compare
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.
👌 thanks for fixing that, way much cleaner! Looks good when the pluralization test starts passing 👀
d02a244
to
461e75a
Compare
@@ -0,0 +1,24 @@ | |||
import { getENV } from "Utils/getENV" | |||
|
|||
// TODO: Serve static assets on the server-side |
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.
@gkartalis @lidimayra what does this mean in this context?
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.
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.
This is actually related to what was commented here: #9797 (comment)
We paired on it a bit, but we never managed to actually make it work, so we decided to move forward without it, as we didn't want it to block the implementation because of this.
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.
What's the benefit of doing that vs simply requiring?
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.
As I understand, this would be an optimization for the future. The server-side rendering would prevent us from loading unnecessary files if we have several JSON files for different locales.
@araujobarret might have the proper context on this.
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.
this would be an optimization for the future
We should probably cross that bridge when we get there and keep it simple for now
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.
Presumably we'd only load this JSON in memory on the server boot — It's not going to affect performance in any meaningful way.
detection: { | ||
order: ["querystring"], | ||
lookupQuerystring: "locale", | ||
}, |
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 won't be using this option? Is there anything else in this config can we remove?
import { NavBar } from "../NavBar" | ||
import { NavBarMobileMenuInboxNotificationCount } from "../NavBarMobileMenu/NavBarMobileMenuInboxNotificationCount" | ||
import { NavBar } from "Components/NavBar/NavBar" | ||
import { NavBarMobileMenuInboxNotificationCount } from "Components/NavBar/NavBarMobileMenu/NavBarMobileMenuInboxNotificationCount" |
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.
Unrelated but I suspect we can just run an autofix for all of these.
461e75a
to
6f54b03
Compare
This fixes the mock so that you don't have to use a monkey patched render etc.