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 post merge Revamp navigation bar #6285

Closed
11 tasks done
lucasbordeau opened this issue Jul 16, 2024 · 3 comments · Fixed by #6297
Closed
11 tasks done

Fix post merge Revamp navigation bar #6285

lucasbordeau opened this issue Jul 16, 2024 · 3 comments · Fixed by #6297
Assignees

Comments

@lucasbordeau
Copy link
Contributor

lucasbordeau commented Jul 16, 2024

Scope & Context

After merge of #6031, we have a lot of small fixes and refactors to do. We left them aside to release a working feature.

Technical inputs

  • Use a recoil state instead of useFindRecordCursorFromFindManyCacheRootQuery
  • Refactor useRecordIdsFromFindManyCacheRootQuery to be used only if there's a cursor in recoil state
  • Refactor useRecordShowPagePagination to be generic with views
  • Merge useScrollToPosition and useScrollRestoration if possible, or refactor accordingly
  • Refactor RecordIndexContainer => move logic outside, useGeCurrent
  • Fix RecordShowPage story (see msw mocks, because requests changed)
  • Rename RecordIndexRecordChip to RecordIdentifierChip
  • Rename isFieldChipDisplay to isFieldIdentifierDisplay
  • Refactor findView and view: any
  • Refactor URL building in shared utils
  • Refactor ROW_HEIGHT extract
Copy link
Contributor

greptile-apps bot commented Jul 16, 2024

Implementation Steps

  1. Fix RecordShowPage story: Update the MSW mocks in RecordShowPage.stories.tsx to match the new request structure.

  2. Merge useScrollToPosition and useScrollRestoration: Combine logic from useScrollToPosition and useScrollRestoration in /src/hooks/ into a single hook, ensuring all scroll restoration functionality is preserved.

  3. Rename RecordIndexRecordChip to RecordIdentifierChip: Rename the component in /src/modules/object-record/record-index/components/RecordIndexRecordChip.tsx to RecordIdentifierChip.tsx and update all imports.

  4. Rename isFieldChipDisplay to isFieldIdentifierDisplay: Rename the variable in /src/modules/object-record/record-field/meta-types/display/components/RelationFromManyFieldDisplay.tsx and update all references.

  5. Refactor RecordIndexContainer: Move logic from /src/modules/object-record/record-index/components/RecordIndexContainer.tsx to a separate hook or utility file. Use useGeCurrent for fetching current state.

  6. Use recoil state instead of useFindRecordCursorFromFindManyCacheRootQuery: Replace useFindRecordCursorFromFindManyCacheRootQuery with a recoil state in /src/modules/object-record/record-index/components/RecordIndexContainer.tsx.

  7. Refactor useRecordIdsFromFindManyCacheRootQuery: Modify useRecordIdsFromFindManyCacheRootQuery in /src/modules/object-record/record-show/hooks/useRecordShowPagePagination.ts to only execute if a cursor exists in the recoil state.

  8. Refactor findView and view: any: Update findView function and view type in /src/modules/object-record/record-show/hooks/useRecordShowPagePagination.ts to use a more specific type.

  9. Refactor useRecordShowPagePagination: Generalize useRecordShowPagePagination in /src/modules/object-record/record-show/hooks/useRecordShowPagePagination.ts to work with different views.

  10. Refactor ROW_HEIGHT extract: Extract ROW_HEIGHT constant to a shared constants file and update all references.

  11. Refactor URL building: Move URL building logic to a shared utility file and update all instances where URLs are constructed.

References

/packages/twenty-front/src/pages/object-record/RecordShowPage.tsx
/packages/twenty-front/src/hooks/useScrollRestoration.ts
/packages/twenty-front/src/modules/object-record/record-index/components/RecordIndexContainer.tsx
/packages/twenty-front/src/modules/object-record/record-show/hooks/useRecordShowPage.ts
/packages/twenty-front/src/modules/object-record/record-show/graphql/operations/factories/findOneRecordForShowPageOperationSignatureFactory.ts
/packages/twenty-front/src/modules/object-record/record-show/components/RecordShowContainer.tsx
/packages/twenty-front/src/modules/object-record/record-show
/packages/twenty-front/src/modules/object-record/record-index/components
/packages/twenty-front/src/modules/object-record/record-show/hooks
/packages/twenty-front/src/modules/object-record/record-show/record-detail-section/components
/packages/twenty-front/src/modules/object-record/record-right-drawer/components
/packages/twenty-front/src/modules/object-record/record-field/meta-types/display/components/RelationFromManyFieldDisplay.tsx

About Greptile

This response provides a starting point for your research, not a precise solution.

Help us improve! Please leave a 👍 if this is helpful and 👎 if it is irrelevant.

Ask Greptile · Edit Issue Bot Settings

@lucasbordeau lucasbordeau moved this from 🆕 New to 🔖 Planned in 🎯 Roadmap & Sprints Jul 16, 2024
@lucasbordeau lucasbordeau moved this from 🔖 Planned to 🏗 In progress in 🎯 Roadmap & Sprints Jul 16, 2024
@lucasbordeau
Copy link
Contributor Author

lucasbordeau commented Jul 17, 2024

@FelixMalfait @charlesBochet

  • Scroll restoration is used on Boards only, and for now this feature is not implemented on Boards, so I just deprecated it.
  • Refactoring the useRecordShowPagePagination to make it more generic without another clear use case feels like premature optimization in this case, I let it like that, I would be glad to do it with more specification or a concrete use case.

Passing the cursor from table seems useless, because we can already have it from the return of the findOne request by using the same query variables than the passed view.

  • we decided to base the behavior on saved views only, thus passing a cursor seems redundant because we already have the view + the recordId

  • we have to request the record id if we don't come from a table page, thus requesting it when we come from the table page seems to be ok conceptually because the common denominator is the view+recordId not the cursor.

  • this way we'll always have the right position, no matter the cursor position (it should be the same but in any case)

@FelixMalfait
Copy link
Member

The assumption that led to passing the parameter was that recomputing the position / converting the sorts into filters would be too expensive, but great if it's not!

@lucasbordeau lucasbordeau moved this from 🏗 In progress to 👀 In review in 🎯 Roadmap & Sprints Jul 17, 2024
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in 🎯 Roadmap & Sprints Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants