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 array edit option #8600

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix array edit option #8600

wants to merge 2 commits into from

Conversation

BKM14
Copy link
Contributor

@BKM14 BKM14 commented Nov 20, 2024

Closes #8578
There was no case to handle a FieldMetadataType.Array and thus an error was thrown every time the edit button was clicked on an Array type.
It has been added now.

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 support for editing Array type fields in the MultiItemFieldInput component, fixing an error that occurred when attempting to edit Array type values.

  • Added FieldMetadataType.Array case in handleEditButtonClick method in /packages/twenty-front/src/modules/object-record/record-field/meta-types/input/components/MultiItemFieldInput.tsx
  • Array type values are now handled similarly to Email types, treating them as strings
  • Consider adding type safety checks when casting Array values to strings

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

Copy link
Contributor

@ehconitin ehconitin left a comment

Choose a reason for hiding this comment

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

Left a comment

@@ -110,6 +110,7 @@ export const MultiItemFieldInput = <T,>({
item = items[index] as PhoneRecord;
setInputValue(item.countryCode + item.number);
break;
case FieldMetadataType.Array:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is there no break condition for this? is the break condition for emails and arrays the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the break statement for Array and Email are the same. The way the Array is being handled in the backend is just like the Email. So I found it better to use the same block as the Email one

@BKM14
Copy link
Contributor Author

BKM14 commented Nov 22, 2024

Hey @ehconitin, could you review this please?

Copy link
Contributor

@ehconitin ehconitin left a comment

Choose a reason for hiding this comment

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

This works, but I think we should have an explicit break for FieldMetadataType.Array. Having an explicit break makes it immediately clear that this is intentional behavior, not an oversight.

@BKM14
Copy link
Contributor Author

BKM14 commented Nov 22, 2024

There is an explicit break for the Array though. If it applies for Email, won't it also apply for Array

@ehconitin
Copy link
Contributor

To clarify - currently, there isn't a separate break for Array. The code falls through from the Array case to the Email case, sharing a single break statement. While they handle the input the same way now, having separate cases with explicit breaks would make the code's intention clearer and easier to maintain.

What if in the future email break conditions change??? Your code will break in that situation :)

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

Successfully merging this pull request may close these issues.

cannot edit array item
2 participants