Skip to content

feat: ability to switch currency format #12542

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

Merged
merged 19 commits into from
Jun 24, 2025

Conversation

MohitAgrawal16
Copy link
Contributor

Fixes #11927

I have added 'format' in the zod schema of currency, and for using it, I am separately passing 'format' to 'currencyDisplay.'
The feature is working correctly.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

Added format switching capability to currency fields, allowing toggle between abbreviated (12M) and full (12,000,000) number displays across the application.

  • Extends currencyFieldDefaultValueSchema.ts with new format field using Zod enum validation, improving type safety
  • Adds format selector component in SettingsDataModelFieldCurrencyForm.tsx with Short/Full options using IconCheckbox
  • Implements format switching in CurrencyDisplay.tsx while maintaining backwards compatibility with default 'short' format
  • Ensures format property is properly propagated through useCurrencyFieldDisplay hook to maintain consistent display behavior

6 files reviewed, 4 comments
Edit PR Review Bot Settings | Greptile

Copy link
Contributor

github-actions bot commented Jun 11, 2025

🚀 Preview Environment Ready!

Your preview environment is available at: http://bore.pub:46276

This environment will automatically shut down when the PR is closed or after 5 hours.

@prastoin prastoin self-requested a review June 11, 2025 14:32
Copy link
Contributor

@prastoin prastoin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @MohitAgrawal16 thanks for your contribution !
Unfortunately this PR seems to introduce several regressions:

  • the currency type select isn't filled with the current selected value
  • unable to create a field metadata currency
  • After changing the currency format only it does not seem possible to save CTA is still disabled

I've pushed few updates to make the format property integration more idiomatic

@MohitAgrawal16
Copy link
Contributor Author

Hello @MohitAgrawal16 thanks for your contribution ! Unfortunately this PR introduces at least one regression regarding the currency type selected value

I've pushed few updates to make the format property integration more idiomatic

Hello @prastoin, thanks for updates. I was also trying to maintain only fieldValue means one prop only, but facing some problems in it, that's why I done it in other way.

@MohitAgrawal16
Copy link
Contributor Author

MohitAgrawal16 commented Jun 11, 2025

Hello @MohitAgrawal16 thanks for your contribution ! Unfortunately this PR seems to introduce several regressions:

  • the currency type select isn't filled with the current selected value
  • unable to create a field metadata currency
  • After changing the currency format only it does not seem possible to save CTA is still disabled

I've pushed few updates to make the format property integration more idiomatic

@prastoin, Oh, so after your updates, the above-mentioned issues are there; I will try to solve them.

@MohitAgrawal16 MohitAgrawal16 requested a review from prastoin June 13, 2025 06:59
@MohitAgrawal16 MohitAgrawal16 changed the title Ability to switch Currency Format feat: ability to switch currency format Jun 16, 2025
@prastoin prastoin force-pushed the feature/switch-currency-format branch from 9bf4fb0 to d14bc5d Compare June 16, 2025 09:52
@prastoin
Copy link
Contributor

@greptileai trigger

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

Updates currency format settings across multiple UI components and field definitions, ensuring consistent format application throughout the application.

  • Extended FieldMetadata.ts types with FieldCurrencyMetadata to support format settings system-wide
  • Added currency format preview handling in getCurrencyFieldPreviewValue.ts by including settings in FieldMetadataItem type

14 files reviewed, 1 comment
Edit PR Review Bot Settings | Greptile

Copy link
Contributor

@prastoin prastoin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting for another core team member review

@prastoin prastoin dismissed their stale review June 23, 2025 13:34

Waiting for another core team member review

@etiennejouan etiennejouan self-assigned this Jun 23, 2025
Copy link
Contributor

@etiennejouan etiennejouan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ! It works great 👏

@MohitAgrawal16
Copy link
Contributor Author

This PR is ready to merge.
Thank you so much, @prastoin, for your key commits and @etiennejouan for reviewing them.

@prastoin prastoin merged commit d0126e2 into twentyhq:main Jun 24, 2025
54 checks passed
Copy link
Contributor

Thanks @MohitAgrawal16 for your contribution!
This marks your 1st PR on the repo. You're top 44% of all our contributors 🎉
See contributor page - Share on LinkedIn - Share on Twitter

Contributions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to switch Currency Format
3 participants