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 field forms #7595

Merged
merged 3 commits into from
Oct 11, 2024
Merged

Fix field forms #7595

merged 3 commits into from
Oct 11, 2024

Conversation

ehconitin
Copy link
Contributor

@lucasbordeau
forms are broken!
revert - #7363
used useRelationSettingsFormInitialValues hook from that commit.
TODO - figure out a way to change the relation name label from singular to plural and vice versa, until it is edited.
related issue - #7355

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

This PR addresses issues with relation field forms, focusing on improving the handling of initial values and field name updates based on relation types.

  • Implemented useRelationSettingsFormInitialValues hook in SettingsDataModelFieldRelationSettingsFormCard.tsx to manage initial values for relation fields
  • Simplified SettingsDataModelFieldIconLabelForm.tsx by removing manual state management for labels and icons
  • Updated SettingsObjectNewFieldConfigure.tsx to use watched form values directly in SettingsDataModelFieldSettingsFormCard
  • Adjusted border color in IconButton.tsx for visual consistency
  • Removed automatic field name updates based on relation type changes in SettingsDataModelFieldRelationForm.tsx, which may affect desired behavior

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

Comment on lines 177 to 179
value={value}
onChange={(newValue) => {
setLabelEditedManually(true);
onChange(newValue);
}}
onChange={onChange}
fullWidth
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: The automatic update of the field name based on relation type changes has been removed. Consider implementing a way to update the field name while still allowing manual edits

Comment on lines 203 to 207
fieldMetadataItem={{
icon: formConfig.watch('icon'),
label: formConfig.watch('label') || 'New Field',
type: fieldType as FieldMetadataType,
}}
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 using a separate variable for the fieldMetadataItem object to improve readability

@lucasbordeau
Copy link
Contributor

@ehconitin Could you create an issue with the relevant changes left to do ?

@lucasbordeau lucasbordeau merged commit d350143 into twentyhq:main Oct 11, 2024
7 checks passed
Copy link

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

Contributions

@ehconitin
Copy link
Contributor Author

follow up issue #7683

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.

3 participants