Conversation
|
|
||
| getLocale(fallback = 'en') { | ||
| const acceptsLanguages = this.getHeader(Header.AcceptLanguage); | ||
| const locales = acceptsLanguages ? acceptsLanguages.split(',') : [fallback]; |
There was a problem hiding this comment.
The split(',') is most-definitely not enough, but want to get a general approval for this PR before getting a more robust parseLanguages thing there.
There was a problem hiding this comment.
This is a good example of something I've seen a bunch in our projects lately. This is (IMO) the wrong place to have this kind of abstraction. It is something most users of the package won't need, or, if they do need it, it is something to do with our usage of projects together, and so the abstraction should live there instead. If we want all consumers of react-i18n to be able to get this feature, we put this logic in that library instead (it can useHeader and do this logic itself). If it's something we want to happen automatically in react-server, it can live in that library instead.
There was a problem hiding this comment.
I don't like this being in react-i18n, it seems very weird for that library to depend on @shopify/react-network
lemonmade
left a comment
lemonmade
left a comment
There was a problem hiding this comment.
Header normalization logic looks good but I am not a fan of this as the place for the locale related info
|
|
||
| getLocale(fallback = 'en') { | ||
| const acceptsLanguages = this.getHeader(Header.AcceptLanguage); | ||
| const locales = acceptsLanguages ? acceptsLanguages.split(',') : [fallback]; |
There was a problem hiding this comment.
This is a good example of something I've seen a bunch in our projects lately. This is (IMO) the wrong place to have this kind of abstraction. It is something most users of the package won't need, or, if they do need it, it is something to do with our usage of projects together, and so the abstraction should live there instead. If we want all consumers of react-i18n to be able to get this feature, we put this logic in that library instead (it can useHeader and do this logic itself). If it's something we want to happen automatically in react-server, it can live in that library instead.
|
|
||
| import {useLazyRef} from '@shopify/react-hooks'; | ||
|
|
||
| import {useRequestHeader, Header} from '@shopify/react-network'; |
There was a problem hiding this comment.
is this a dependency we want to add to @shopify/react-i18n? It seems weird to me that react-i18n would have this
| return [i18n, shareTranslationsComponent.current]; | ||
| } | ||
|
|
||
| export function useAcceptLanguages(fallback = 'en') { |
There was a problem hiding this comment.
I feel like this should be part of @shopify/react-network, or some other package that handles the concerns of specifically using @shopify/react-i18n with @shopify/react-network. Maybe even @shopify/react-server?
|
|
||
| getLocale(fallback = 'en') { | ||
| const acceptsLanguages = this.getHeader(Header.AcceptLanguage); | ||
| const locales = acceptsLanguages ? acceptsLanguages.split(',') : [fallback]; |
There was a problem hiding this comment.
I don't like this being in react-i18n, it seems very weird for that library to depend on @shopify/react-network
cartogram
changed the title
getLocale to react-networkuseAcceptLanguages() hook
|
@lemonmade @TheMallen re-did this as a stand-alone hook in |
3 participants
Description
Fixes (issue #931)
Type of change
Checklist