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

Replace entityId by recordId in the front end. #6355

Merged
merged 5 commits into from
Aug 7, 2024

Conversation

ehconitin
Copy link
Contributor

Hey @lucasbordeau, I replaced every entityId with recordId in the frontend.

Some clarifications:

  1. At this line, I changed recordId to selectedRecordId because that component has entityId defined in code at this line which was to be changed to recordId. To avoid repeated constants, I changed the arguments to selectedRecordIds.

  2. At the following links:

    It seems to be the tests. As I can see, it is checking for both objectFilterDropdownSelectedEntityIdState and objectFilterDropdownSelectedRecordIdsState. Since entityIds are supposed to be removed and recordedId state is already being checked, I commented out all the entityidState code. If they are to be removed or uncommented, please let me know, and I will do as expected.

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

Replaced all instances of entityId with recordId across the frontend codebase to standardize identifier terminology.

  • Updated CalendarEventDetails.tsx and EventFieldDiffValue.tsx to use recordId in FieldContext.Provider.
  • Modified useFieldContext.tsx to replace entityId with recordId in context and state management.
  • Commented out objectFilterDropdownSelectedEntityIdState references in useFilterDropdown.ts and useFilterDropdownStates.ts.
  • Updated test files like useFilterDropdown.test.tsx and usePersistField.test.tsx to reflect the new recordId terminology.
  • Ensured consistency in hooks like useClearField.ts, useInitDraftValueV2.ts, and useIsFieldEmpty.ts by replacing entityId with recordId.

30 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

Copy link
Contributor

@lucasbordeau lucasbordeau left a comment

Choose a reason for hiding this comment

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

  • Could you create a util getRecordFieldInputId that returns ${recordId}-${fieldDefinition?.metadata?.fieldName} from params.
  • Could you clean recordId: recordId => recordId,

);
});
});
// it('should set objectFilterDropdownSelectedEntityId', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok we should get rid of objectFilterDropdownSelectedEntityId state, but we should rewrite this test for objectFilterDropdownSelectedRecordIdsState

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So what should I in this case, keep test cases as it is and create a new issue to rewrite the tests?
What do u suggest?

Copy link
Contributor

@lucasbordeau lucasbordeau left a comment

Choose a reason for hiding this comment

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

Nice job !

@ehconitin
Copy link
Contributor Author

ehconitin commented Jul 22, 2024

@lucasbordeau

  • Could you create a util getRecordFieldInputId that returns ${recordId}-${fieldDefinition?.metadata?.fieldName} from params.

Yes, Ill work on it.

  • Could you clean recordId: recordId => recordId,

I am sorry but could u please give an example on where this should be cleaned up.

@lucasbordeau
Copy link
Contributor

I am sorry but could u please give an example on where this should be cleaned up.

In usePersistField

recordStoreFamilySelector({ recordId: recordId, fieldName }),

To :

recordStoreFamilySelector({ recordId, fieldName }),

@ehconitin
Copy link
Contributor Author

Got it, my bad should have cleaned in the first place. But will do and let you know.

@ehconitin
Copy link
Contributor Author

@lucasbordeau

  • Could you create a util getRecordFieldInputId that returns ${recordId}-${fieldDefinition?.metadata?.fieldName} from params. - Done
  • Could you clean recordId: recordId => recordId. - Done

Could u please review and let me know if there are more changes to be made.

Thanks :)

Copy link
Member

@FelixMalfait FelixMalfait left a comment

Choose a reason for hiding this comment

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

@lucasbordeau is off this week so I will merge like this to avoid further conflicts. Great work thanks again!

@FelixMalfait FelixMalfait merged commit a794131 into twentyhq:main Aug 7, 2024
3 of 8 checks passed
Copy link

github-actions bot commented Aug 7, 2024

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

Contributions

@ehconitin ehconitin deleted the my-feature-branch branch August 7, 2024 07:04
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