-
Notifications
You must be signed in to change notification settings - Fork 154
feat: i18nify SDK integrated for Amount
component
#2003
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
Changes from 6 commits
31a3e05
b9c23c8
a13df00
82d6490
d26b19e
3ff6855
e21eb0f
8221c55
184b8fb
67a1172
67ca36c
b983083
b2a390d
cba99d7
5e8686d
dd61843
82bdb6b
d9462bd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
--- | ||
"@razorpay/blade": minor | ||
--- | ||
|
||
feat: Added internationalization in Amount component via i18nify. | ||
References | ||
- **i18nify-js:** https://www.npmjs.com/package/@razorpay/i18nify-js | ||
- **i18nify-react:** https://www.npmjs.com/package/@razorpay/i18nify-react | ||
|
||
How to update ? | ||
1. Install i18nify peer dependency `yarn add @razorpay/i18nify-js` | ||
kamleshchandnani marked this conversation as resolved.
Show resolved
Hide resolved
|
||
2. _[Optional]_: Install i18nify-react peer dependency to manage state effectively `yarn add @razorpay/i18nify-react` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We were discussing about breaking change internally. We decided to have this as minor release and add its changelog into v11 https://github.com/razorpay/blade/blob/master/packages/blade/upgrade-v11.md#amount because we very recently had a breaking release which people are yet to migrate to. I think we'll have A bit risky to rely on browser locale for existing Amount components since our default so far has been en-IN behaviour so things might break for product if something like this changes - https://github.com/razorpay/blade/pull/2003/files#r1482641498 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes makes sense There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
regarding this, should we explicitly mark |
||
3. Install latest Blade `yarn add @razorpay/blade@latest` | ||
tarun-khanna marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -269,7 +269,9 @@ | |
"typescript": "4.9", | ||
"typescript-transform-paths": "3.4.6", | ||
"@types/body-scroll-lock": "3.1.0", | ||
"ramda": "0.29.1" | ||
"ramda": "0.29.1", | ||
"@razorpay/i18nify-js": "^1.4.0", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we use pinned dependency, can you pin it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we need to install both? ideally i18nify-react shall have i18nify-js as dependency otherwise as end consumer if the dependency versions go out of sync it will be difficult to communicate this to devs. so if someone is installing i18nify-react it shall install i18nify-js by default right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, it not should not install Here for Blade, we needed A similar discussion happened with Blade team on this thread too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
this is the issue i see. how will we prevent consumers from updating the one and not the other? |
||
"@razorpay/i18nify-react": "^4.0.0" | ||
}, | ||
"peerDependencies": { | ||
"react": ">=18", | ||
|
@@ -283,7 +285,8 @@ | |
"react-native-svg": "^12.3.0", | ||
"react-native-gesture-handler": "^2.9.0", | ||
"@gorhom/bottom-sheet": "^4.4.6", | ||
"@gorhom/portal": "^1.0.14" | ||
"@gorhom/portal": "^1.0.14", | ||
"@razorpay/i18nify-js": "^1.4.0" | ||
kamleshchandnani marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}, | ||
"peerDependenciesMeta": { | ||
"react-native": { | ||
|
@@ -298,7 +301,9 @@ | |
"@storybook/**/react": "18.2.0", | ||
"react-dom": "18.2.0", | ||
"react": "18.2.0", | ||
"styled-components": "^5" | ||
"styled-components": "^5", | ||
"@types/react": "17.0.38", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we have types react as 17 here while our actual react version is 18? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Had added this while testing |
||
"@types/react-native": "0.72.2" | ||
}, | ||
"bundlemon": { | ||
"files": [ | ||
|
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.
Ideally, this should be treated as a breaking change since users need manual intervention (installing peerDep) to upgrade.
We can make an exception for this PR, to avoid a major release. I'll discuss this with the team.
cc @anurag-rzp @saurabhdaware @kamleshchandnani @chaitanyadeorukhkar
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 can do it as minor, should be fine
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.
minor should be fine I believe since its just additional installation step and shouldn't require consumers to change anything in code as such. And even if it breaks, it breaks on development itself