feat: i18nify SDK integrated for `Amount` component #2003

tarun-khanna · 2024-02-02T09:05:21Z

Description

This PR integrates i18nify (Atlas) SDK in Blade's Amount Component.

Changes

Modified internal logic within Amount component to format number by utilising i18nify APIs instead.
1.1 Amount formatting (commas and spaces) will be now driven by locale state set in i18nify SDK. Earlier, Blade supported only 2 types for formatting en-IN and en-US. If currency was INR, it chose en-IN otherwise en-US.

Before	After

1.2. CurrencyList to be now retrieved from getCurrencyList API of i18nify.

1.3. Denomination symbol will now be localized based on the locale set. Earlier, the abbreviations were localized only for India and US. If the currency was INR, it localized it to K, lac, Crores otherwise K, M, B..

Before	After

1.4. Denomination position will also be decided by i18nify based on the locale state set in the SDK. Earlier, denomination symbol was simply placed opposite to the placement of currency symbol.

Before	After

1.5. Currency symbol will also be based on the user's locale and not the native language to give better readability to that user.

Before	After

Updated Unit test cases for new contracts of functions that have changed.
Added a new story to display Amount in different locales

Note => We are currently exploring the appropriate short form for denoting 1000 in the Indian locale—whether it should be 1K or 1T. Once a decision is reached, I will add the necessary adjustments in this PR before merging.
Update => We have concluded to keep it T as it is a standard norm and followed by other global applications too. For more details, refer here.

References

Additional Information

Component Checklist

Update Component Status Page
Perform Manual Testing in Other Browsers
Add KitchenSink Story
Add Interaction Tests (if applicable)
Add changeset

changeset-bot · 2024-02-02T09:05:24Z

🦋 Changeset detected

Latest commit: d9462bd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@razorpay/blade	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2024-02-02T09:06:36Z

✅ PR title follows Conventional Commits specification.

packages/blade/package.json

packages/blade/src/components/Amount/Amount.tsx

…istry changes

codesandbox-ci · 2024-02-05T08:56:18Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit d9462bd:

Sandbox	Source
razorpay/blade: basic	Configuration

snitin315

Thanks for the PR, overall looks good. Can you also add the following:

A story that showcases i18n support for multiple locales.
Changeset with upgrade guide

packages/blade/src/components/Amount/Amount.tsx

snitin315 · 2024-02-05T23:30:57Z

Are these changes backward compatible with the old Amount component? For instance, if a project is using the Amount component which hasn't integrated the i18nify SDK, would it cause any issues? Is just installing the peerDep @razorpay/i18nify-js enough to upgrade or they will need some additional configuration steps as well?

tarun-khanna · 2024-02-06T07:25:57Z

Are these changes backward compatible with the old Amount component? For instance, if a project is using the Amount component which hasn't integrated the i18nify SDK, would it cause any issues? Is just installing the peerDep @razorpay/i18nify-js enough to upgrade or they will need some additional configuration steps as well?

Adding peerDeps is mandatory to updrade to new Amount component, otherwise it will break.
Additionaly, the external project can install i18nify-react dependency to manage state more effectively in React ecosystem.

tarun-khanna · 2024-02-06T10:26:34Z

Thanks for the PR, overall looks good. Can you also add the following:

A story that showcases i18n support for multiple locales.

Changeset with upgrade guide

Have added the story and changeset with guide. Refer PR description for details. @snitin315

snitin315 · 2024-02-06T11:17:55Z

.changeset/purple-flies-end.md

@@ -0,0 +1,13 @@
+---
+"@razorpay/blade": minor


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

We can do it as minor, should be fine

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

packages/blade/src/components/Amount/Amount.stories.tsx

snitin315 · 2024-02-06T11:40:03Z

@tarun-khanna Also you need to update snapshots, you can run yarn test:react -u & yarn test:react-native -u to update them.

…8nify

snitin315 · 2024-02-07T09:23:08Z

@tarun-khanna There are still some react-native snapshots failing, can you update them too?

packages/blade/upgrade-v11.md

saurabhdaware

Looks good to me

.changeset/purple-flies-end.md

kamleshchandnani · 2024-02-09T06:49:43Z

packages/blade/package.json

@@ -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",


we use pinned dependency, can you pin it?

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?

No, it not should not install i18nify-js automatically if someone install i18nify-react.
As we want to maintain a single copy of i18nify-js in consumer apps for state management, i18nify-js is added as a peer dependency even for i18nify-react. So you will always have to install i18nify-js separately if you want i18nify-react.

Here for Blade, we needed I18nProvider and associated context state, so I had to install i18nify-react as a separate dev dependency but not peer dependency.
Additionally, the end consumers won't lead to a out of sync issue, as these packages are released always in sync in our mono-repo setup. If a consumer installs a latest copy of i18nify-react, they will have to update latest version of i18nify-js as well.

A similar discussion happened with Blade team on this thread too.

If a consumer installs a latest copy of i18nify-react, they will have to update latest version of i18nify-js as well.

this is the issue i see. how will we prevent consumers from updating the one and not the other?

packages/blade/package.json

kamleshchandnani · 2024-02-09T06:52:51Z

packages/blade/package.json

@@ -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",


why do we have types react as 17 here while our actual react version is 18?

Had added this while testing i18nify-react in Blade/examples/basic app.
As Blade uses @types/react: 17.0.38 and i18nify-react uses "@types/react": "^18.0.0",. It was leading to some conflicts in the types.
Let me recheck this, will remove this if not required.

kamleshchandnani · 2024-02-09T06:57:36Z

packages/blade/src/components/Amount/Amount.tsx

-  amountLineHeights,
-  currencyPositionMapping,
-} from './amountTokens';
+import type { CurrencyCodeType } from '@razorpay/i18nify-js/currency';


consumers using i18nify react will need to install i18nify-js if they have to use these types?

To use i18nify-react, you always have to install i18nify-js as it is a core package which maintains state.
It is added as a peer dependency as we want to keep a single copy of i18nify-js in the consumer app.

kamleshchandnani · 2024-02-09T07:08:58Z

.changeset/purple-flies-end.md

+
+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 ?**


we shall also update the installation guide with installation of i18nify. its located at packages/blade/docs/guides/Installation.stories.mdx

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

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?

kamleshchandnani · 2024-02-09T07:10:50Z

@tarun-khanna the stories page for amount shall also be updated with the i18nify installation guides and some information. Right the information is outdated on this page

kamleshchandnani · 2024-02-09T07:26:53Z

@tarun-khanna how would I as a user know that the currency-indicator prop is tied to user's browser locale? for eg: my default locale is en-GB and when I toggle this property from storybook to currency-code and select currency as USD it shows me US$123 which is correct but confusing to me as a dev as I was expecting $.

To fix this we shall do 2 things

Put notes in the amount output and show the browser's defualt locale in the storybook.
In the currencyIndicator prop we shall add this description to the TS types that this is tied to locale set via i18nify and it defaults to the browser's default(and print the browser's default locale in the table as well within the description)

kamleshchandnani · 2024-02-09T07:31:27Z

packages/blade/src/components/Amount/Amount.stories.tsx

+                title={`${item.country}(${item.locale})`}
+                value={item.locale}
+                onClick={({ name }) => {
+                  setI18nState?.({ locale: name });


@tarun-khanna this is setting the context at a global amount's level ideally we shall scope it within this story itself?

Why?
I opened the amount story page i.e default Docs page and saw the currency in some default locale then I navigated to the locale story and was playing around with it and then went back to the Docs page of amount and it was pointing to the locale I set under the locale story which was misleading as there's no information mentioned about that

Setting a state by I18nProvider's setI18nState will always set it globally for i18nify-js. We decided to architect our library in this fashion so to deal with out of sync state issues between sidebar, header, body and other elements as discussed in our earlier meetings.

To deal with this,

We can introduce an internal locale prop hidden to consumers.

Then on locale dropdown click, We don't have to call setI18nState but can maintain an internal locale state which will be passed to the Amount instance used in this story.

We can then pass locale prop to i18nify-js formatNumber api which accepts this locale param for overriding global state.

Thoughts ? @kamleshchandnani

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

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.
Attaching MOM here

cc @kamleshchandnani

tarun-khanna · 2024-02-09T09:22:42Z

@tarun-khanna the stories page for amount shall also be updated with the i18nify installation guides and some information. Right the information is outdated on this page

@kamleshchandnani Can we connect on this ? Would like to know your thoughts on how to showcase this in the Amount story.

tarun-khanna · 2024-02-09T09:25:21Z

To fix this we shall do 2 things

Put notes in the amount output and show the browser's defualt locale in the storybook.

In the currencyIndicator prop we shall add this description to the TS types that this is tied to locale set via i18nify and it defaults to the browser's default(and print the browser's default locale in the table as well within the description)

Makes sense @kamleshchandnani , Are you looking to send these changes in this PR itself or can we send this separately ?

kamleshchandnani · 2024-02-09T10:47:00Z

To fix this we shall do 2 things

Put notes in the amount output and show the browser's defualt locale in the storybook.

In the currencyIndicator prop we shall add this description to the TS types that this is tied to locale set via i18nify and it defaults to the browser's default(and print the browser's default locale in the table as well within the description)

Makes sense @kamleshchandnani , Are you looking to send these changes in this PR itself or can we send this separately ?

yes since everything its related directly to amount component

kamleshchandnani · 2024-02-09T10:45:37Z

.changeset/purple-flies-end.md

+
+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 ?**


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?

kamleshchandnani · 2024-02-12T06:31:19Z

packages/blade/src/components/Amount/Amount.stories.tsx


 const Page = (): React.ReactElement => {
  return (
    <StoryPageWrapper
      componentName="Amount"
      componentDescription="Amounts are used to show small amount of color coded metadata, which are ideal for getting user attention."
-      note="This component only displays the provided value in the specified currency, it does not perform any currency conversion."
+      note="This component only displays the provided value in the specified currency with the formatting capabilities enabled by @razorpay/i18nify-js (https://www.npmjs.com/package/@razorpay/i18nify-js), it does not perform any currency conversion."


Suggested change

note="This component only displays the provided value in the specified currency with the formatting capabilities enabled by @razorpay/i18nify-js (https://www.npmjs.com/package/@razorpay/i18nify-js), it does not perform any currency conversion."

note="This component only displays the provided value in the specified currency with the formatting capabilities enabled by @razorpay/i18nify-react, it does not perform any currency conversion."

kamleshchandnani · 2024-02-12T06:37:30Z

packages/blade/src/components/Amount/Amount.stories.tsx

@@ -37,6 +38,22 @@ const Page = (): React.ReactElement => {
        export default App;
        `}
      </Sandbox>
+      <Box marginBottom="spacing.10">
+        <Text>
+          The Amount component automatically formats numbers based on the user's browser locale


you can use the propname propsDescription on the StoryPageWrapper component and put this information there you can remove links and only mention i18nify-react. ignore the js since that will go away with the approach we discussed last time

can you update the messaging a bit? for more details of what? please refer to the documentation of what?

kamleshchandnani · 2024-02-12T06:39:33Z

packages/blade/docs/guides/Installation.stories.mdx

-   ```
-2. Follow [this guide](#-install-fonts) to install the fonts.
+```shell
+yarn add @razorpay/blade styled-components@5.3.11


move this shell block before styled components and put all the dependencies including i18nify in one shell block itself as we have done in React native section below and then you can use bullt points to describe all the dependencies together

Co-authored-by: Nitin Kumar <snitin315@gmail.com> Co-authored-by: Kamlesh Chandnani <kamlesh.chandnani@gmail.com>

[feat]: i18nify SDK integrated for Amount component

31a3e05

tarun-khanna requested review from chaitanyadeorukhkar and kamleshchandnani as code owners February 2, 2024 09:05

tarun-khanna changed the title ~~[feat]: i18nify SDK integrated for Amount component~~ feat: i18nify SDK integrated for Amount component Feb 2, 2024

snitin315 reviewed Feb 2, 2024

View reviewed changes

packages/blade/package.json Outdated Show resolved Hide resolved

packages/blade/src/components/Amount/Amount.tsx Outdated Show resolved Hide resolved

tarun-khanna force-pushed the feat/i18nify branch from 8e3c73b to 2ade7cc Compare February 5, 2024 07:31

[chore]: setting up npmrc in install deps

b9c23c8

tarun-khanna force-pushed the feat/i18nify branch from 2ade7cc to b9c23c8 Compare February 5, 2024 07:39

[chore]: syntax fix in install deps action

a13df00

tarun-khanna force-pushed the feat/i18nify branch from e97b9cf to a13df00 Compare February 5, 2024 07:59

[chore]: i18nify-js installed from npm registry + reverted github reg…

82d6490

…istry changes

snitin315 reviewed Feb 5, 2024

View reviewed changes

packages/blade/src/components/Amount/Amount.tsx Show resolved Hide resolved

tarun-khanna and others added 2 commits February 6, 2024 15:44

[chore]: Amount story added for diff locales

d26b19e

Create purple-flies-end.md

3ff6855

tarun-khanna requested a review from snitin315 February 6, 2024 10:28

snitin315 reviewed Feb 6, 2024

View reviewed changes

packages/blade/src/components/Amount/Amount.stories.tsx Outdated Show resolved Hide resolved

packages/blade/src/components/Amount/Amount.stories.tsx Outdated Show resolved Hide resolved

snitin315 assigned saurabhdaware Feb 6, 2024

snitin315 added the Review - L2 Second level of review label Feb 6, 2024

tarun-khanna added 2 commits February 6, 2024 22:11

[chore]: Story UI fix + snapshots updated

e21eb0f

Merge branch 'feat/i18nify' of github.com:razorpay/blade into feat/i1…

8221c55

…8nify

[chore]: react-native snapshots updated

184b8fb

tarun-khanna dismissed snitin315’s stale review via 9c547ef February 8, 2024 11:36

[chore]: updated v11 migration guide for Amount component

b983083

tarun-khanna force-pushed the feat/i18nify branch from 9c547ef to b983083 Compare February 8, 2024 11:38

saurabhdaware reviewed Feb 8, 2024

View reviewed changes

packages/blade/upgrade-v11.md Outdated Show resolved Hide resolved

snitin315 reviewed Feb 8, 2024

View reviewed changes

packages/blade/upgrade-v11.md Show resolved Hide resolved

[chore]: updated migration guide

b2a390d

tarun-khanna force-pushed the feat/i18nify branch from 62b5085 to b2a390d Compare February 8, 2024 12:13

saurabhdaware previously approved these changes Feb 8, 2024

View reviewed changes

tarun-khanna requested a review from snitin315 February 8, 2024 12:29

snitin315 previously approved these changes Feb 8, 2024

View reviewed changes

kamleshchandnani reviewed Feb 9, 2024

View reviewed changes

[chore]: PR fixes

cba99d7

tarun-khanna dismissed stale reviews from snitin315 and saurabhdaware via cba99d7 February 9, 2024 10:06

kamleshchandnani and others added 2 commits February 9, 2024 16:18

Merge branch 'master' into feat/i18nify

5e8686d

[chore]: updates on the guide and installation

dd61843

kamleshchandnani requested changes Feb 12, 2024

View reviewed changes

tarun-khanna added 2 commits February 12, 2024 15:08

[chore]: PR fixes for documentation

82bdb6b

[chore]: minor update in docs

d9462bd

kamleshchandnani approved these changes Feb 12, 2024

View reviewed changes

kamleshchandnani changed the title ~~feat: i18nify SDK integrated for Amount component~~ feat: i18nify SDK integrated for Amount component Feb 12, 2024

kamleshchandnani merged commit f104cbf into master Feb 12, 2024
14 checks passed

kamleshchandnani deleted the feat/i18nify branch February 12, 2024 10:05

anuraghazra pushed a commit that referenced this pull request Apr 9, 2024

feat: i18nify SDK integrated for Amount component (#2003)

9618495

Co-authored-by: Nitin Kumar <snitin315@gmail.com> Co-authored-by: Kamlesh Chandnani <kamlesh.chandnani@gmail.com>


		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 ?

	note="This component only displays the provided value in the specified currency with the formatting capabilities enabled by @razorpay/i18nify-js (https://www.npmjs.com/package/@razorpay/i18nify-js), it does not perform any currency conversion."
	note="This component only displays the provided value in the specified currency with the formatting capabilities enabled by @razorpay/i18nify-react, it does not perform any currency conversion."

feat: i18nify SDK integrated for Amount component #2003

feat: i18nify SDK integrated for Amount component #2003

Conversation

tarun-khanna commented Feb 2, 2024 • edited Loading

Description

Changes

Additional Information

Component Checklist

changeset-bot bot commented Feb 2, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Feb 2, 2024 • edited Loading

codesandbox-ci bot commented Feb 5, 2024 • edited Loading

snitin315 left a comment • edited Loading

Choose a reason for hiding this comment

snitin315 commented Feb 5, 2024

tarun-khanna commented Feb 6, 2024

tarun-khanna commented Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snitin315 commented Feb 6, 2024

snitin315 commented Feb 7, 2024

saurabhdaware left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamleshchandnani commented Feb 9, 2024 • edited Loading

kamleshchandnani commented Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamleshchandnani Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarun-khanna commented Feb 9, 2024

tarun-khanna commented Feb 9, 2024

kamleshchandnani commented Feb 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamleshchandnani Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarun-khanna Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

feat: i18nify SDK integrated for `Amount` component #2003

feat: i18nify SDK integrated for `Amount` component #2003

tarun-khanna commented Feb 2, 2024 •

edited

Loading

changeset-bot bot commented Feb 2, 2024 •

edited

Loading

github-actions bot commented Feb 2, 2024 •

edited

Loading

codesandbox-ci bot commented Feb 5, 2024 •

edited

Loading

snitin315 left a comment •

edited

Loading

tarun-khanna commented Feb 6, 2024 •

edited

Loading

kamleshchandnani commented Feb 9, 2024 •

edited

Loading

kamleshchandnani commented Feb 9, 2024 •

edited

Loading

kamleshchandnani Feb 9, 2024 •

edited

Loading

kamleshchandnani Feb 12, 2024 •

edited

Loading

tarun-khanna Feb 12, 2024 •

edited

Loading