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

feat: record group fetch more #8868

Merged
merged 8 commits into from
Dec 5, 2024
Merged

Conversation

magrinj
Copy link
Member

@magrinj magrinj commented Dec 4, 2024

Fix #8756

This PR is adding a load more button when we're grouping by a field in table view.
Only 8 records are printed at first, and a click on Load more is needed to show more records.

Screenshot 2024-12-04 at 11 54 15 AM Screenshot 2024-12-04 at 11 54 23 AM

@magrinj magrinj marked this pull request as ready for review December 4, 2024 10:57
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

This PR replaces infinite scroll with a "Load more" button for grouped records in table view, limiting initial fetch to 8 records per group and allowing manual loading of additional records.

  • Added recordIndexShouldFetchMoreRecordsByGroupComponentState and recordIndexHasFetchedAllRecordsByGroupComponentState Recoil family states to manage pagination per group
  • Modified useLoadRecordIndexTable to limit initial fetch to 8 records when grouping is active
  • Added new RecordTableRecordGroupSectionLoadMore component to render "Load more" button below each group
  • Added RecordTableActionRow component for consistent styling of action rows with icons and text
  • Updated RecordTableRecordGroupBodyEffect to handle group-specific record loading and state management

9 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings | Greptile

@@ -84,6 +84,8 @@ export const useFindManyParams = (
...recordGroupFilter,
},
orderBy,
// If we have a current record group definition, we only want to fetch 8 records by page
...(currentRecordGroupDefinition ? { limit: 8 } : {}),
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider making the 8 record limit configurable via a constant rather than hardcoding the value

Comment on lines 88 to 91
fetchMoreRecords().finally(() => {
setShouldFetchMoreRecords(false);
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Missing error handling for failed fetch operations. The state should be reset even if the fetch fails.

Comment on lines +38 to +42
type RecordTableActionRowProps = {
LeftIcon: IconComponent;
text: string;
onClick?: (event?: React.MouseEvent<HTMLTableRowElement>) => void;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: onClick handler should be required since this is an interactive component - remove the optional '?' from the type definition

@@ -108,8 +108,8 @@ export const RecordTableRecordGroupSection = () => {
/>
<StyledTotalRow>{recordIdsByGroup.length}</StyledTotalRow>
Copy link
Contributor

Choose a reason for hiding this comment

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

style: recordIdsByGroup.length may not reflect total records when paginated. Consider showing total count instead

Comment on lines +111 to +112
<StyledEmptyTd />
<StyledEmptyTd />
Copy link
Contributor

Choose a reason for hiding this comment

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

style: empty td elements should have aria-hidden for better accessibility

Copy link
Member

@charlesBochet charlesBochet left a comment

Choose a reason for hiding this comment

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

Try (5 min timebox):

  • useLazyFindMany query in useLoadRecordIndexTable
  • in NoBodyEffect and in Effect, make a useEffect to trigger the lazy query
  • in RecordTableRecordGroupLoadMore, import useLoardRecordIndexTable again and trigger fetchMore

@charlesBochet charlesBochet merged commit 7ab00a4 into main Dec 5, 2024
16 checks passed
@charlesBochet charlesBochet deleted the feat/record-group-fetch-more branch December 5, 2024 09:51
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.

Record group table "Load more" button
2 participants