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

Issue6335: RecordInlineCell tree refactor with RecordInlineCellContext #6537

Merged
merged 14 commits into from
Aug 12, 2024

Conversation

hansol-y
Copy link
Contributor

@hansol-y hansol-y commented Aug 5, 2024

Fixes #6335

This pull request is for issue #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 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!

Copy link

github-actions bot commented Aug 5, 2024

Fails
🚫

node failed.

Log

�[31mError: �[39m RequestError [HttpError]: You have exceeded a secondary rate limit. Please wait a few minutes before you try again. If you reach out to GitHub Support for help, please include the request ID 6407:2C0E26:ED6D05:1CE99F0:66B9E198.
    at /home/runner/work/twenty/twenty/node_modules/�[4m@octokit�[24m/request/dist-node/index.js:86:21
�[90m    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)�[39m {
  status: �[33m403�[39m,
  response: {
    url: �[32m'https://api.github.com/search/issues?q=is%3Apr%20author%3Ahansol-y%20is%3Aclosed%20repo%3Atwentyhq%2Ftwenty&per_page=2&page=1'�[39m,
    status: �[33m403�[39m,
    headers: {
      �[32m'access-control-allow-origin'�[39m: �[32m'*'�[39m,
      �[32m'access-control-expose-headers'�[39m: �[32m'ETag, Link, Location, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Used, X-RateLimit-Resource, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval, X-GitHub-Media-Type, X-GitHub-SSO, X-GitHub-Request-Id, Deprecation, Sunset'�[39m,
      connection: �[32m'close'�[39m,
      �[32m'content-encoding'�[39m: �[32m'gzip'�[39m,
      �[32m'content-security-policy'�[39m: �[32m"default-src 'none'"�[39m,
      �[32m'content-type'�[39m: �[32m'application/json; charset=utf-8'�[39m,
      date: �[32m'Mon, 12 Aug 2024 10:19:04 GMT'�[39m,
      �[32m'referrer-policy'�[39m: �[32m'origin-when-cross-origin, strict-origin-when-cross-origin'�[39m,
      server: �[32m'github.com'�[39m,
      �[32m'strict-transport-security'�[39m: �[32m'max-age=31536000; includeSubdomains; preload'�[39m,
      �[32m'transfer-encoding'�[39m: �[32m'chunked'�[39m,
      vary: �[32m'Accept-Encoding, Accept, X-Requested-With'�[39m,
      �[32m'x-content-type-options'�[39m: �[32m'nosniff'�[39m,
      �[32m'x-frame-options'�[39m: �[32m'deny'�[39m,
      �[32m'x-github-api-version-selected'�[39m: �[32m'2022-11-28'�[39m,
      �[32m'x-github-media-type'�[39m: �[32m'github.v3; format=json'�[39m,
      �[32m'x-github-request-id'�[39m: �[32m'6407:2C0E26:ED6D05:1CE99F0:66B9E198'�[39m,
      �[32m'x-ratelimit-limit'�[39m: �[32m'30'�[39m,
      �[32m'x-ratelimit-remaining'�[39m: �[32m'30'�[39m,
      �[32m'x-ratelimit-reset'�[39m: �[32m'1723458004'�[39m,
      �[32m'x-ratelimit-resource'�[39m: �[32m'search'�[39m,
      �[32m'x-ratelimit-used'�[39m: �[32m'1'�[39m,
      �[32m'x-xss-protection'�[39m: �[32m'0'�[39m
    },
    data: {
      documentation_url: �[32m'https://docs.github.com/free-pro-team@latest/rest/overview/rate-limits-for-the-rest-api#about-secondary-rate-limits'�[39m,
      message: �[32m'You have exceeded a secondary rate limit. Please wait a few minutes before you try again. If you reach out to GitHub Support for help, please include the request ID 6407:2C0E26:ED6D05:1CE99F0:66B9E198.'�[39m
    }
  },
  request: {
    method: �[32m'GET'�[39m,
    url: �[32m'https://api.github.com/search/issues?q=is%3Apr%20author%3Ahansol-y%20is%3Aclosed%20repo%3Atwentyhq%2Ftwenty&per_page=2&page=1'�[39m,
    headers: {
      accept: �[32m'application/vnd.github.v3+json'�[39m,
      �[32m'user-agent'�[39m: �[32m'octokit-rest.js/18.12.0 octokit-core.js/3.6.0 Node.js/18.20.4 (linux; x64)'�[39m,
      authorization: �[32m'token [REDACTED]'�[39m
    },
    request: { hook: �[36m[Function: bound bound register]�[39m }
  }
}
danger-results://tmp/danger-results-1b89e8d3.json

Generated by 🚫 dangerJS against 9d013f8

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

Refactored RecordInlineCell components to use RecordInlineCellContext for state management, eliminating prop drilling and improving maintainability.

  • Added RecordInlineCellContext.tsx to manage state and props centrally.
  • Updated packages/twenty-front/src/modules/object-record/record-inline-cell/components/RecordInlineCell.tsx to use context values.
  • Modified packages/twenty-front/src/modules/object-record/record-inline-cell/components/RecordInlineCellContainer.tsx to retrieve props from context.
  • Adjusted packages/twenty-front/src/modules/object-record/record-inline-cell/components/RecordInlineCellDisplayMode.tsx to use context.
  • Refactored packages/twenty-front/src/modules/object-record/record-inline-cell/components/RecordInlineCellValue.tsx to access context values.

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

@bosiraphael
Copy link
Contributor

@lucasbordeau

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, I just added some minor fixes.

@lucasbordeau lucasbordeau merged commit b6202fe into twentyhq:main Aug 12, 2024
9 of 11 checks passed
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.

Refactor RecordInlineCell tree with a Context to avoid props drilling
4 participants