-
Notifications
You must be signed in to change notification settings - Fork 274
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
Add pathPrefix to i18n and use storefront.i18n
#330
Conversation
This comment has been minimized.
This comment has been minimized.
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.
LGTM, though I like the idea of always having a default i18n
to make the type more easy to work with.
I can change that in this PR. @jplhomer @wizardlyhel do you see any concern with that? |
love this, great stuff @frandiox |
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.
<3
@@ -5,7 +5,7 @@ import {Await, useLoaderData} from '@remix-run/react'; | |||
import {ProductSwimlane, FeaturedCollections, Hero} from '~/components'; | |||
import {COLLECTION_CONTENT_FRAGMENT, PRODUCT_CARD_FRAGMENT} from '~/data'; | |||
import {getHeroPlaceholder} from '~/lib/placeholders'; | |||
import {getLocaleFromRequest} from '~/lib/utils'; |
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.
should we delete this util?
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.
It's still needed in server.ts
to extract the locale from request
the first time.
We were calling the
getLocaleFromRequest
in many loaders but we already have the same information instorefront.i18n
. This PR starts using that instead. Also, it addspathPrefix
to the types of thei18n
with a default value.Thoughts?
On a side note, it's unfortunate that
storefront.i18n
is possiblyundefined
, even though we are passing a value increateStorefrontClient
but we can't infer it, so we always need to access it asstorefront.i18n!
. Should we add a default value of{language: 'EN', country: 'US'}
tostorefront.i18n
to improve the type? Would this default value be a problem?