-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Allow input and display of floats for Number fields #7340
Conversation
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.
PR Summary
This pull request implements the ability to input and display float values for Number fields in the Twenty application. Here's a concise summary of the major changes:
- Added 'settings' property with 'decimals' field to FieldMetadataItem and FieldDefinition types
- Introduced new SettingsDataModelFieldNumberDecimalsInput component for configuring decimal places
- Updated NumberFieldDisplay and NumberDisplay components to support decimal formatting
- Modified useNumberField hook to handle float values instead of just integers
- Replaced integer-specific utility functions with more flexible number handling functions
- Added unit tests for new number casting functions in cast-as-number-or-null.test.ts
- Updated server-side field metadata settings interface to include 'decimals' property
21 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings
settings | ||
defaultValue | ||
options | ||
settings |
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.
syntax: Duplicate 'settings' field in the response. Remove one instance to avoid redundancy.
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.
agreed with greptile !
settings | ||
options | ||
settings |
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.
syntax: Duplicate 'settings' field. Remove one instance.
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.
same
<StyledCounterInnerContainer> | ||
<StyledExampleText>Example: {exampleValue}</StyledExampleText> | ||
<StyledCounterControlsIcons> | ||
<StyledInfoButton variant="tertiary" Icon={IconInfoCircle} /> |
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.
logic: The info button lacks an onClick handler or tooltip to provide additional information
if (castedNumber === null) { | ||
onChange(MIN_VALUE); | ||
return; |
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.
style: Consider setting the value to an empty string or undefined instead of MIN_VALUE when input is invalid
it(`should throw if trying to cast a float string to an integer`, () => { | ||
expect(castAsNumberOrNull('9.9')).toBe(9.9); | ||
}); |
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.
logic: The test description says 'should throw' but the expectation is not to throw. Consider updating the description to 'should cast a float string to a float'.
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.
nice work, a few things to address still!
settings | ||
defaultValue | ||
options | ||
settings |
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.
agreed with greptile !
settings | ||
options | ||
settings |
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.
same
...tings/data-model/fields/forms/number/components/SettingsDataModelFieldNumberDecimalInput.tsx
Show resolved
Hide resolved
...r/src/engine/metadata-modules/field-metadata/interfaces/field-metadata-settings.interface.ts
Show resolved
Hide resolved
...ules/settings/data-model/fields/forms/number/components/SettingsDataModelFieldNumberForm.tsx
Outdated
Show resolved
Hide resolved
...tings/data-model/fields/forms/number/components/SettingsDataModelFieldNumberDecimalInput.tsx
Outdated
Show resolved
Hide resolved
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.
Approved!
Allowed mysef to push some changes notably to keep on displaying numbers US-style (ex 800000 should be displayed 800,000)
Just waiting on confirmation from @Bonapara on the minimal number of decimals we want to show (not sure if 0 or settings.decimals)
thanks!
Log
|
### Description - We added a decimal field for a Number Field type in the settings - We updated the Number Field type create a form with decimals input - We are not implementing the dropdown present on the Figma because it seems not related ### Demo <https://www.loom.com/share/18a8d4b712a14f6d8b66806764f8467f?sid=3fc79b46-ae32-46e3-8635-d0eee02e53b2> Fixes twentyhq#6987 --------- Co-authored-by: gitstart-twenty <[email protected]> Co-authored-by: Marie Stoppa <[email protected]>
Description
Demo
https://www.loom.com/share/18a8d4b712a14f6d8b66806764f8467f?sid=3fc79b46-ae32-46e3-8635-d0eee02e53b2
Fixes #6987