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

Record Page Navigation Arrows Cause Unnecessary skeleton loading #6367

Merged
merged 3 commits into from
Jul 29, 2024

Conversation

ehconitin
Copy link
Contributor

@ehconitin ehconitin commented Jul 22, 2024

@Bonapara
Issue #6325

  • Desired Behavior
    The navigation should always be visible. Clicking on a Next/Previous arrow should immediately increment the number without switching to the skeleton loading step.
    Done

Please let me know what do you think about this approach.

Thanks :)

2024-07-23.00-23-59.mp4

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

The pull request addresses unnecessary skeleton loading when navigating between records by updating state management and removing conditional rendering logic.

  • Modified packages/twenty-front/src/modules/object-record/record-show/hooks/useRecordShowPagePagination.ts to manage totalCount state dynamically using useState and useEffect.
  • Removed skeleton loading component and conditional rendering logic in packages/twenty-front/src/modules/ui/layout/page/PageHeader.tsx.
  • Eliminated isLoadingPagination state from packages/twenty-front/src/pages/object-record/RecordShowPage.tsx to streamline navigation.

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

@Bonapara
Copy link
Member

Bonapara commented Jul 23, 2024

Thanks @ehconitin! Could the "Chevron icon button" remain active so a user can click it multiple times? For instance, if I'm on record 8 and want to access record 11, I would like to click the chevron down icon button three times without waiting for records 9 and 10 to load first.

By the way @ehconitin, are you on Discord? I would love to schedule a quick call to thank you for your contributions 🙏

@ehconitin
Copy link
Contributor Author

Hey @Bonapara, let me get back to you regarding chevron down icon button being active.

Yes I am on discord, would love to have a chat with you.
username - neeko2853

@ehconitin
Copy link
Contributor Author

@Bonapara

  • Could the "Chevron icon button" remain active so a user can click it multiple times? For instance, if I'm on record 8 and want to access record 11, I would like to click the chevron down icon button three times without waiting for records 9 and 10 to load first.

I looked into how this can be achieved. Could it be done on counting the number of clicks and calling the function which navigates the records prev or next that many times?
what do u think about this approach?
This is the only way I could think of without making a major changes to the useRecordShowPagePagination hook. which is a complex change and would need some help with.
:)

@Bonapara
Copy link
Member

Hi @ehconitin, thanks for your answer. I think you're right; this requires a deeper refactor by @lucasbordeau. Instead of fetching the next ID when the page loads, we should fetch the next five and the previous five at once. This way, we can handle navigation without waiting for the page to load each time we want to move to the next record.

@lucasbordeau WDYT? Should I create an issue? I think we can merge this PR in the meantime

@martmull
Copy link
Contributor

@Bonapara there is a bug when we refresh the page, the display 1 of 13 companies turns into companies (13), not related to that PR but I wanted you to know

@martmull martmull self-assigned this Jul 29, 2024
Copy link
Contributor

@martmull martmull left a comment

Choose a reason for hiding this comment

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

@Bonapara asked for a complete removal of the loader
EDIT: Forget it, wrong PR

@martmull martmull merged commit 6bc7622 into twentyhq:main Jul 29, 2024
11 checks passed
Copy link

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 fixed-issue-6325 branch August 6, 2024 06:14
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.

4 participants