Reactive form preview #8663
Reactive form preview #8663
Conversation
Welcome!
Hello there, congrats on your first PR! We're excited to have you contributing to this project. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Implemented real-time preview updates for number fields in the Data Model Editor, focusing on immediate UI feedback when modifying field settings.
- Fixed percentage display in
NumberFieldDisplay.tsxby removing incorrect *100 multiplication - Added settings watch functionality in
SettingsDataModelFieldNumberSettingsFormCard.tsxfor live preview updates - Modified
getFieldPreviewValue.tsto handle number fields with a default preview value of 2000 - Extended
fieldMetadataItemtype to include settings property for better type safety - Simplified field preview logic by removing redundant utility functions while maintaining core functionality
3 file(s) reviewed, 6 comment(s)
Edit PR Review Bot Settings | Greptile
| const value = type === 'percentage' | ||
| ? `${formatNumber(numericValue, decimals)}%` |
There was a problem hiding this comment.
logic: Percentage values should be multiplied by 100 before display. The old code correctly did Number(fieldValue) * 100, but this was removed. This will cause percentages to display incorrectly (e.g. 0.5 will show as '0.5%' instead of '50%').
|
|
||
| if (!fieldValue) return <NumberDisplay value={null} />; | ||
|
|
||
| const numericValue = Number(fieldValue); |
There was a problem hiding this comment.
style: Consider handling invalid number conversion (NaN) cases here
| fieldMetadataItem={fieldMetadataItem} | ||
| fieldMetadataItem={{ | ||
| ...fieldMetadataItem, | ||
| settings: settings || fieldMetadataItem.settings, |
There was a problem hiding this comment.
logic: The fallback to fieldMetadataItem.settings could be undefined if both settings are undefined, potentially causing runtime issues
| fieldMetadataItem: Pick< | ||
| FieldMetadataItem, | ||
| 'type' | 'defaultValue' | 'settings' | ||
| >; |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| isDefined(fieldTypeConfig.exampleValue) | ||
| ) { | ||
| return fieldTypeConfig.exampleValue; | ||
| const fieldTypeConfig = getSettingsFieldTypeConfig(fieldMetadataItem.type as SettingsFieldType); |
There was a problem hiding this comment.
logic: Type assertion to SettingsFieldType may be unsafe if fieldMetadataItem.type is not a valid SettingsFieldType
| const fieldTypeConfig = getSettingsFieldTypeConfig(fieldMetadataItem.type as SettingsFieldType); | ||
|
|
||
| if (fieldMetadataItem.type === FieldMetadataType.Number) { | ||
| return fieldTypeConfig.exampleValue || 2000; |
There was a problem hiding this comment.
style: Magic number 2000 should be defined as a constant with meaningful name
|
Thanks for your work. I will be reviewing it. Could you first pull the latest version of main branch : I was working on a small fix today : https://github.com/twentyhq/twenty/pull/8684/files ? 🙏 |
|
@guillim I have synced it now |
| ? formatNumber(Number(fieldValue), decimals) | ||
| : null; | ||
|
|
||
| if (!fieldValue) return <NumberDisplay value={null} />; |
There was a problem hiding this comment.
Nice one. Faster and consistent with our way of returning instead of handling else statement
| import { isDefined } from '~/utils/isDefined'; | ||
| import { FieldMetadataType } from '~/generated-metadata/graphql'; | ||
|
|
||
| const DEFAULT_NUMBER_PREVIEW_VALUE = 2000; |
There was a problem hiding this comment.
Here, you define something that is specific to Number.
However, this preview helper is agnositc (neutral) of its children.
Consequence is that if you look at an adress preview, it is now broken. Maybe you can find a way to make it ok for all kind of fieldMetadataItem types ?
Description
This PR fixes issues with field previews not updating immediately when settings are changed in the Data Model Editor. The changes affect number field types, ensuring that the preview updates in real-time as settings are modified.
Fixed Issues
Recording
https://www.loom.com/share/14a30f67266d4a08a694c759ae06b0f3?sid=c0de35ef-9982-438b-b822-94ed106f6891
Fixes #8663