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

Refactor RecordInlineCell tree with a Context to avoid props drilling #6335

Closed
lucasbordeau opened this issue Jul 19, 2024 · 1 comment · Fixed by #6537
Closed

Refactor RecordInlineCell tree with a Context to avoid props drilling #6335

lucasbordeau opened this issue Jul 19, 2024 · 1 comment · Fixed by #6537
Assignees
Labels

Comments

@lucasbordeau
Copy link
Contributor

lucasbordeau commented Jul 19, 2024

Scope & Context

All the hierarchy of RecordInlineCell components

Technical inputs

We currently use a lot of props drilling, with components slicing props types of more than 10 properties, it's difficult to maintain and should be better placed in a context at the top of the hierarchy since those values are all readonly and defined when the component is mounted.

Example in RecordInlineCellValue :

type RecordInlineCellValueProps = Pick<
  RecordInlineCellContainerProps,
  | 'displayModeContent'
  | 'customEditHotkeyScope'
  | 'editModeContent'
  | 'editModeContentOnly'
  | 'isDisplayModeFixHeight'
  | 'disableHoverEffect'
  | 'readonly'
  | 'buttonIcon'
  | 'loading'
  | 'showLabel'
  | 'label'
  | 'isCentered'
>;

We shouldn't have even one props because everything is passed at the top of the hierarchy.

Please note that it shouldn't be put in FieldContext as it is not in the domain of a Field but of RecordInlineCell which are two different abstractions.

@hansol-y
Copy link
Contributor

hansol-y commented Jul 20, 2024

Hi @lucasbordeau, can you please assign me for this issue? I'm interested in this one!

Edit: I worked on this issue and made PR6359

lucasbordeau added a commit that referenced this issue Aug 12, 2024
#6537)

Fixes [#6335](#6335)

This pull request is for issue
[#6335](#6335): Refactor
RecordInlineCell tree with a Context to avoid props drilling. For the
refactoring, this PR made changes as below:

- Created new script RecordInlineCellContext.tsx: Defining a context to
pass in useContext()
- Updated RecordInlineCell.tsx: Passing the necessary props as context
values, wrapping with RecordInlineCellContext.Provider
- Updated RecordInlineCellContainer.tsx: Passing the props to
RecordInlineContainer as RecordInlineCellContext
- Updated RecordInlineCellDisplayMode.tsx: retrieves values from
useRecordInlineCellContext instead of directly assigning them
- RecordInlineCellValue.tsx: Removed values passed through
<RecordInlineCellDisplayMode> as they are now retrieved through
useRecordInlineCellContext + Removed the null check for
RecordInlineCellContextProps.

Using RecordInlineCellContext, I believe the context goes to the top of
the hierarchy and passed to the required layers without going through
several layers. However, please let me know if I understood the issue
incorrectly or it is not solved properly.

Thank you in advance for your review!

---------

Co-authored-by: Lucas Bordeau <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants