Skip to content
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

fix-percentage #8684

Merged
merged 2 commits into from
Nov 22, 2024
Merged

fix-percentage #8684

merged 2 commits into from
Nov 22, 2024

Conversation

guillim
Copy link
Contributor

@guillim guillim commented Nov 22, 2024

Following previous release, a suggestion was noticed by @martmull . Here is a suggested fix.

Before :
image

After :
Screenshot 2024-11-22 at 14 56 07

@guillim guillim requested a review from martmull November 22, 2024 13:57
@guillim guillim self-assigned this Nov 22, 2024
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 percentage symbol display functionality to number fields in the settings form when the field type is set to 'percentage', improving visual clarity for percentage value representation.

  • Modified packages/twenty-front/src/modules/settings/data-model/fields/forms/number/components/SettingsDataModelFieldNumberForm.tsx to include % symbol in preview display
  • Added conditional rendering logic to append % for percentage type fields
  • Note: TODO comment regarding maxValue needs attention in future updates

1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

@@ -69,7 +69,7 @@ export const SettingsDataModelFieldNumberForm = ({
<SettingsOptionCardContentCounter
Icon={IconDecimal}
title="Number of decimals"
description={`Example: ${(1000).toFixed(count)}`}
description={`Example: ${(1000).toFixed(count)} ${type === 'percentage' ? '%' : ''}`}
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider formatting the example number (1000) differently for percentage display - 1000% might be confusing to users

@@ -69,7 +69,7 @@ export const SettingsDataModelFieldNumberForm = ({
<SettingsOptionCardContentCounter
Icon={IconDecimal}
title="Number of decimals"
description={`Example: ${(1000).toFixed(count)}`}
description={`Example: ${(1000).toFixed(count)} ${type === 'percentage' ? '%' : ''}`}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
description={`Example: ${(1000).toFixed(count)} ${type === 'percentage' ? '%' : ''}`}
description={`Example: ${(type === 'percentage' ? 99 : 1000).toFixed(count)} ${type === 'percentage' ? '%' : ''}`}

Copy link
Contributor

Choose a reason for hiding this comment

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

To match design
image

@guillim guillim requested a review from martmull November 22, 2024 14:14
@martmull martmull enabled auto-merge (squash) November 22, 2024 14:15
@martmull martmull merged commit cb5a0c1 into main Nov 22, 2024
19 checks passed
@martmull martmull deleted the fix-percentage-missing branch November 22, 2024 14:20
Copy link

Thanks @guillim for your contribution!
This marks your 8th PR on the repo. You're top 4% 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
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants