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: infinite scroll history tab #125

Merged
merged 1 commit into from
Aug 30, 2024
Merged

Conversation

samsiegart
Copy link
Contributor

@samsiegart samsiegart commented Aug 29, 2024

Copy link

cloudflare-workers-and-pages bot commented Aug 30, 2024

Deploying dapp-econ-gov with  Cloudflare Pages  Cloudflare Pages

Latest commit: 4b97341
Status: ✅  Deploy successful!
Preview URL: https://d2e9b8b5.dapp-econ-gov.pages.dev
Branch Preview URL: https://infinite-scroll-history.dapp-econ-gov.pages.dev

View logs

Copy link

github-actions bot commented Aug 30, 2024

Cloudflare deployment logs are available here

@@ -77,5 +77,6 @@
"vite": "^3.2.7",
"vite-tsconfig-paths": "^3.5.1",
"vitest": "^0.25.3"
}
},
"packageManager": "[email protected]+sha512.a6b2f7906b721bba3d67d4aff083df04dad64c399707841b7acf00f6b133b7ac24255f2652fa22ae3534329dc6180534e98d17432037ff6fd140556e2bb3137e"
Copy link
Member

Choose a reason for hiding this comment

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

sha seems excessive. how was this added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think corepack just added it when I did yarn install. Should I just remove this diff from the PR?

Copy link
Member

Choose a reason for hiding this comment

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

good to leave if that's how it works now

src/components/HistoryPanel.tsx Outdated Show resolved Hide resolved
src/components/HistoryPanel.tsx Outdated Show resolved Hide resolved
/>
</motion.div>
));
const receivedItems =
Copy link
Member

Choose a reason for hiding this comment

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

let's try to avoid nested ternaries. (surprised we don't have a lint rule)

would this work?

const receivedItems = dataLoaded ?
  instanceStatus === 'received' && questionsWithAnswers.map(
  )
  : (
    <div className="italic text-center mt-16">No questions yet.</div>
  );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not quite as suggested but just changed it to not use a nested ternary. FWIW I'm not super against nested ternaries personally, I think they can be convenient when writing jsx

Comment on lines 101 to 103
outcome={
aData?.question === qData.questionHandle ? aData : undefined
}
Copy link
Member

Choose a reason for hiding this comment

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

these come from the same tuple so why wouldn't they match? If you're just handling no outcome, can QuestionDetails be responsible for that?

Suggested change
outcome={
aData?.question === qData.questionHandle ? aData : undefined
}
outcome={aData}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yea good catch. This wasn't part of my change but just reduced this down to outcome={aData}

}
const loader = (
<div ref={loaderRef}>
{!dataLoaded && (
Copy link
Member

Choose a reason for hiding this comment

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

consider a ternary for clarity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

if (paginationSize && curPageSize >= paginationSize) {
setData(items);
await fetchNextP;
fetchNextP = new Promise<void>(res =>
Copy link
Member

Choose a reason for hiding this comment

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

this is a little hard for me to follow. res resolves when the effect triggered by setFetchNextPage completes?

Could it work with a counter of pages fetched? That might be a clearer state to trigger

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried doing something more iterative with iterable[Symbol.asyncIterator]() but it was more messy and I gave up working through bugs. Just added some comments even though this is a little tricky with the promises.

Copy link
Contributor Author

@samsiegart samsiegart left a comment

Choose a reason for hiding this comment

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

Thanks for review. PTAL

@@ -77,5 +77,6 @@
"vite": "^3.2.7",
"vite-tsconfig-paths": "^3.5.1",
"vitest": "^0.25.3"
}
},
"packageManager": "[email protected]+sha512.a6b2f7906b721bba3d67d4aff083df04dad64c399707841b7acf00f6b133b7ac24255f2652fa22ae3534329dc6180534e98d17432037ff6fd140556e2bb3137e"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think corepack just added it when I did yarn install. Should I just remove this diff from the PR?

src/components/HistoryPanel.tsx Outdated Show resolved Hide resolved
src/components/HistoryPanel.tsx Outdated Show resolved Hide resolved
/>
</motion.div>
));
const receivedItems =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not quite as suggested but just changed it to not use a nested ternary. FWIW I'm not super against nested ternaries personally, I think they can be convenient when writing jsx

Comment on lines 101 to 103
outcome={
aData?.question === qData.questionHandle ? aData : undefined
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yea good catch. This wasn't part of my change but just reduced this down to outcome={aData}

}
const loader = (
<div ref={loaderRef}>
{!dataLoaded && (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

if (paginationSize && curPageSize >= paginationSize) {
setData(items);
await fetchNextP;
fetchNextP = new Promise<void>(res =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried doing something more iterative with iterable[Symbol.asyncIterator]() but it was more messy and I gave up working through bugs. Just added some comments even though this is a little tricky with the promises.

@samsiegart samsiegart requested a review from turadg August 30, 2024 18:33
@samsiegart samsiegart merged commit b86c514 into main Aug 30, 2024
3 checks passed
@samsiegart samsiegart deleted the infinite-scroll-history branch August 30, 2024 19:00
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 slow loading of history tab
2 participants