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: fix pagination for fills #5228

Merged
merged 2 commits into from
Dec 19, 2024
Merged

feat: fix pagination for fills #5228

merged 2 commits into from
Dec 19, 2024

Conversation

anxolin
Copy link
Contributor

@anxolin anxolin commented Dec 18, 2024

Summary

Fixes the pagination for fills

Before
https://explorer-dev-git-add-double-pagination-cowswap.vercel.app/orders/0xc0688a39e9ce5ee01c49786867357811c3fcf86e27eefb072e80c9417d731bf3a53a13a80d72a855481de5211e7654fabdfe352664fd9408?tab=fills

image

After
Screenshot at Dec 18 12-39-17

Test

test for order 0xc0688a39e9ce5ee01c49786867357811c3fcf86e27eefb072e80c9417d731bf3a53a13a80d72a855481de5211e7654fabdfe352664fd9408

Copy link

vercel bot commented Dec 18, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
cosmos ✅ Ready (Inspect) Visit Preview Dec 18, 2024 0:56am
cowfi ✅ Ready (Inspect) Visit Preview Dec 18, 2024 0:56am
explorer-dev ✅ Ready (Inspect) Visit Preview Dec 18, 2024 0:56am
sdk-tools ✅ Ready (Inspect) Visit Preview Dec 18, 2024 0:56am
swap-dev ✅ Ready (Inspect) Visit Preview Dec 18, 2024 0:56am
widget-configurator ✅ Ready (Inspect) Visit Preview Dec 18, 2024 0:56am

@@ -5,7 +5,7 @@ import { Trade } from 'api/operator'
import { TableState, TableStateSetters } from '../../../../explorer/components/TokensTableWidget/useTable'

type CommonState = {
trades: Trade[]
data: Trade[]
isLoading: boolean
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shoom3301 this one was hard to debug, cause the error comes from some unsafety casting done in the pagination.

I could see the context being injected:
Screenshot at Dec 18 12-30-13

However, because there's this type casting here:
image

We don't get compile errors. Basically, was assuming there will be something called data, but instead injected something called trades

This PR is about fixing the issue to get it out, but maybe we should add to the technical debt this too

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it, thanks for the information.
This context should definetely be refactored to jotai atom, at least in terms of code-base consistency.

Copy link

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

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

Yaaay! Thank you!
I'd add the pagination to the bottom of the table as well, but I'm already happy with the current one!

Copy link
Collaborator

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

To be honest, I don't get this fix.
Seems to me it's just renaming a variable (trades -> data)??

@anxolin
Copy link
Contributor Author

anxolin commented Dec 19, 2024

To be honest, I don't get this fix.
Seems to me it's just renaming a variable (trades -> data)??

that's what it is. The code there is fragile. The better fix will be to bring typesafety there. But no capacity, so I just wanted to bring the pagination back

@anxolin anxolin merged commit 9563df4 into main Dec 19, 2024
11 of 13 checks passed
@anxolin anxolin deleted the fix-pagination-fills branch December 19, 2024 20:15
@github-actions github-actions bot locked and limited conversation to collaborators Dec 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants