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: merge course labels across pages #541

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ishita778
Copy link
Contributor

@ishita778 ishita778 commented Mar 2, 2025

Copy link
Collaborator

@doprz doprz left a comment

Choose a reason for hiding this comment

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

Please make sure that all checks are passing

@ishita778 ishita778 requested a review from doprz March 2, 2025 04:14
@Samathingamajig Samathingamajig self-requested a review March 3, 2025 00:56
Copy link
Collaborator

@Samathingamajig Samathingamajig left a comment

Choose a reason for hiding this comment

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

The current version will always remove the first row of a page (except for the first page, which doesn't go through the addRows function), because a header row has row.course === null, which is true on the first iteration, and then each course object with non-null values have unique places in memory so !== will always be true.

When you go to the page for unique numbers 00000-99999, you can see where this logic fails.

The first 2 removals of "the first header on the page" work because they happen to be the same as the previous label

image image

However, for this example, the page boundary lies cleanly between two different courses, so this label should be preserved

image

despite the fact that it should be preserved, this PR deletes that label (it should be between 00875 and 00880

image

You'll need to query on the row.element dom node if course === null to get the course title, then check it against the previous.

To preserve data across multiple calls to addRows, you should use useRef(https://stackoverflow.com/a/56456055/12101554) because you want to store state but you don't need it to trigger a rerender when you change lastCourse.

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.

[fix] Merge together the same course across pagination
3 participants