-
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 12 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,23 @@ | ||
--- | ||
'@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 | ||
|
||
**What changes ?** | ||
|
||
1. The `<Amount />` component will now automatically format numbers based on the user's browser locale. For example, `<Amount value={123456.789} currency="INR">` will render `₹1,23,456.79` for browsers with the `en-IN` default locale, whereas it will render `₹123,456.79` for browsers with the `en-US` locale. | ||
|
||
2. If you want to enable users to change the locale of your page, add the `@razorpay/i18nify-react` package and wrap your app inside the `I18nProvider`. Utilize the `setI18nState` utility to modify the locale. For more details, please refer to the [documentation](https://www.npmjs.com/package/@razorpay/i18nify-react). | ||
|
||
3. Additionally, if you prefer to maintain a fixed locale for your page and amount component, enclose your app within `<I18nProvider initData={{locale: 'locale-you-want'}}>..`. For more details, please refer to the [documentation](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": [ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,16 +1,22 @@ | ||
import type { StoryFn, Meta } from '@storybook/react'; | ||
import { Title } from '@storybook/addon-docs'; | ||
import { getCurrencyList } from '@razorpay/i18nify-js/currency'; | ||
import { I18nProvider, useI18nContext } from '@razorpay/i18nify-react'; | ||
import { useState } from 'react'; | ||
import type { AmountProps } from './Amount'; | ||
import { Amount as AmountComponent } from './Amount'; | ||
import type { AmountHeadingProps, AmountDisplayProps, AmountBodyProps } from './amountTokens'; | ||
import { currencyIndicatorMapping } from './amountTokens'; | ||
import { getStyledPropsArgTypes } from '~components/Box/BaseBox/storybookArgTypes'; | ||
import BaseBox from '~components/Box/BaseBox'; | ||
import { Sandbox } from '~utils/storybook/Sandbox'; | ||
import { Display, Text } from '~components/Typography'; | ||
import StoryPageWrapper from '~utils/storybook/StoryPageWrapper'; | ||
import { Box } from '~components/Box'; | ||
import { objectKeysWithType } from '~utils/objectKeysWithType'; | ||
import { ActionList, ActionListItem } from '~components/ActionList'; | ||
import { SelectInput } from '~components/Input/DropdownInputTriggers'; | ||
import { Dropdown, DropdownOverlay } from '~components/Dropdown'; | ||
import { Divider } from '~components/Divider'; | ||
|
||
const Page = (): React.ReactElement => { | ||
return ( | ||
|
@@ -176,7 +182,7 @@ HumanizeSuffix.args = { | |
HumanizeSuffix.storyName = 'Humanize Suffix'; | ||
|
||
const AmountCurrencyTemplate: StoryFn<typeof AmountComponent> = (args) => { | ||
const values = Object.keys(currencyIndicatorMapping); | ||
const values = Object.keys(getCurrencyList()); | ||
|
||
return ( | ||
<BaseBox justifyContent="flex-start" maxHeight="300px" overflowY="auto"> | ||
|
@@ -217,3 +223,98 @@ StrikeThrough.args = { | |
isStrikethrough: true, | ||
}; | ||
StrikeThrough.storyName = 'Strike Through'; | ||
|
||
// TODO: Replace below with i18nify getDefaultLocales API | ||
saurabhdaware marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const localeList = [ | ||
{ | ||
country: 'India', | ||
locale: 'en-IN', | ||
}, | ||
{ | ||
country: 'USA', | ||
locale: 'en-US', | ||
}, | ||
{ | ||
country: 'Malaysia', | ||
locale: 'ms-MY', | ||
}, | ||
{ | ||
country: 'France', | ||
locale: 'fr-FR', | ||
}, | ||
{ | ||
country: 'Germany', | ||
locale: 'de-DE', | ||
}, | ||
]; | ||
|
||
const I18nAmountWrapper = (args: AmountProps): JSX.Element => { | ||
const { setI18nState } = useI18nContext(); | ||
const [currency, setCurrency] = useState('INR'); | ||
|
||
return ( | ||
<> | ||
<AmountComponent {...args} currency={currency as AmountProps['currency']} /> | ||
<Divider marginY="spacing.4" marginTop="spacing.8" /> | ||
<Dropdown selectionType="single"> | ||
<SelectInput label="Select currency" /> | ||
<DropdownOverlay> | ||
<ActionList> | ||
{Object.keys(getCurrencyList()).map((value) => ( | ||
<ActionListItem | ||
key={value} | ||
title={value} | ||
value={value} | ||
onClick={({ name }) => { | ||
setCurrency(name); | ||
}} | ||
/> | ||
))} | ||
</ActionList> | ||
</DropdownOverlay> | ||
</Dropdown> | ||
<Divider marginY="spacing.4" /> | ||
<Dropdown selectionType="single"> | ||
<SelectInput label="Select locale" /> | ||
<DropdownOverlay> | ||
<ActionList> | ||
{localeList.map((item) => ( | ||
<ActionListItem | ||
key={item.locale} | ||
title={`${item.country}(${item.locale})`} | ||
value={item.locale} | ||
onClick={({ name }) => { | ||
setI18nState?.({ locale: name }); | ||
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. @tarun-khanna this is setting the context at a global amount's level ideally we shall scope it within this story itself? Why? 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. Setting a state by I18nProvider's To deal with this,
Thoughts ? @kamleshchandnani 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. Okay i'm saying keep it simple from demonstration point of view because right now there's no co-relation to the end consumer that changes in one story is affecting other. what I'm suggesting is wrap another i18n context around just the locale story so the state changes is only affected to that story and not goes all the stories of amount component. No changes in implementation, just how the story is written 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. As discussed earlier, there is no finalised approach here that will resolve this issue so we will be addressing this issue in the next PR. The current one can still go as it is. |
||
}} | ||
/> | ||
))} | ||
</ActionList> | ||
</DropdownOverlay> | ||
</Dropdown> | ||
</> | ||
); | ||
}; | ||
|
||
const I18nAmountTemplate: StoryFn<typeof AmountComponent> = (args) => { | ||
return ( | ||
<I18nProvider> | ||
<BaseBox justifyContent="flex-start" minHeight="300px" overflowY="auto"> | ||
<BaseBox | ||
display="flex" | ||
alignItems="baseline" | ||
paddingRight="spacing.3" | ||
paddingTop="spacing.3" | ||
flexDirection="column" | ||
> | ||
<I18nAmountWrapper {...args} /> | ||
</BaseBox> | ||
</BaseBox> | ||
</I18nProvider> | ||
); | ||
}; | ||
|
||
export const I18nAmount = I18nAmountTemplate.bind({}); | ||
I18nAmount.args = { | ||
...defaultArgs, | ||
}; | ||
I18nAmount.storyName = 'Amount in diff locales'; |
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 shall also update the installation guide with installation of i18nify. its located at
packages/blade/docs/guides/Installation.stories.mdx
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.
Sure, I was planning to send this in a separate PR, but we can incorporate these changes into this one.
I'll need your assist to finalize the content and format for inclusion here. @kamleshchandnani
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.
yes please add it in this PR itself because everything is related to Amount in this context. Let me know what exactly do you need help with?
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.
updated.