Skip to content
This repository was archived by the owner on Apr 4, 2022. It is now read-only.

Create buttons component to paginate user orders table #694

Merged
merged 9 commits into from
Oct 5, 2021

Conversation

henrypalacios
Copy link
Contributor

@henrypalacios henrypalacios commented Sep 29, 2021

Summary

Closes #687, #684

  • Create a button component with arrows.
  • Use a component in top tab header with a legend of page count.
  • Handle button click to paginate the next or previous page.

To Test

  1. (687-Pager) -> Go to https://pr694--gpui.review.gnosisdev.com/rinkeby/address/0xFF714b8b0e2700303eC912BD40496C3997ceEa2b/

  2. (684-EmptyTable) Go to https://pr694--gpui.review.gnosisdev.com/address/0xb6BAd41ae76A11D10f7b0E664C5007b908bC77C9/

Reference

@henrypalacios henrypalacios added app:Explorer Explorer App Protofire task to the protofire team labels Sep 29, 2021
@github-actions
Copy link

@elena-zh
Copy link

Hi @henrypalacios , changes LGTM!
I have found several minor issues, but I reported them in separate tasks that might be addressed later:
#698 - enhancement to add navigation to the 1st/last pages, add Qty of lines
#699 - some blinking issues with pagination
#700 - issue with number of lines

Btw, pagination is missing in the mobile view, but I hope, that it will be addressed under #693

Copy link
Contributor

@alongoni alongoni left a comment

Choose a reason for hiding this comment

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

Looks good Henry!

I agree with Elena's comment to keep it in the radar for future improvements.

Copy link
Contributor

@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.

Some minor comments on the code. Overall quite good 👍

Regarding the behaviour, echoing comments I made in the other issues I feel like make part of this change:

  1. Changing the rows per page should reset position back to page 1
  2. When not on page 1, rows should not be added/removed, only update what's already visible. Now it causes rows to "shift" down with background updates. Probably the easiest is to disable background re-fresh when not on first page.
  3. Not sold on showing the count of orders (51 - 100). Or maybe I'm missing the page number. What about adding that in between the page controls

For follow ups/other issues

  1. Continuing the second page refresh issues. Keep track of what was the last order is the previous table and continue from there, avoiding showing the same order again.
  2. Also on that, we could add a indicator like "new orders, click here to refresh" sort of thing. Low priority, though.
  3. Add page controls to bottom of table as well - not part of the design but in practice I see the need

All in all, with 1 and 2 added it'll be good to merge IMO. We can discuss the rest and address in follow up tasks/PRs if we agree.

What do you think?

@henrypalacios
Copy link
Contributor Author

henrypalacios commented Oct 1, 2021

Thanks for the feedback @alfetopito .

I agree, I wanted to make progress on 1 and 2. But I got stuck on 2. Because I didn't realize that I was testing on Rinkeby at https://cowswap.exchange/#/swap 🤕. I'll work on point 2.

@alfetopito
Copy link
Contributor

Thanks for the feedback @alfetopito .

I agree, I wanted to make progress on 1 and 2. But I got stuck on 2. Because I didn't realize that I was testing on Rinkeby at https://cowswap.exchange/#/swap face_with_head_bandage. I'll work on point 2.

Just to be clear, Rinkeby works on any stage.
The issue in your case is that you were creating backend PROD orders.
Using anything other than PROD will give you STAGING backend orders.
So, barn.cowswap.exchange, cowswap.dev.gnosisdev.com or even a local version will work for you

And, this happened to me yesterday as well when I was testing it 😅

@henrypalacios
Copy link
Contributor Author

@elena-zh I have complied with points 1 and 2. Closes #700

  1. Changing the rows per page should reset position back to page 1
  2. When not on page 1, rows should not be added/removed, only update what's already visible. Now it causes

I tried point 3. but it didn't look very good, maybe @alongoni can help shape that idea.

@alfetopito @anxolin
What do you think about adding later a dependency like react-table? it could help in the future when adding the other TradesTable and other filters.

@alfetopito I have problems with the coverage of the useTable hook, but I see that the '@testing-library/react-hooks' library is not installed. Can we add it to cover the hook test?

@elena-zh
Copy link

elena-zh commented Oct 4, 2021

Hey @henrypalacios !
Case 1 #700 LGTM!

However, now I have started to face order list updating issues. When I on the 1st page, and place an order, at some moment order list stops to be updating. As a result, I do not see an 'Open' order until I refresh the page or flip pages:
refresh

In addition, an order status might not be updated
cancelled

@henrypalacios
Copy link
Contributor Author

@elena-zh the second case I could not reproduce, so I think it had to do with the first.

@alfetopito I've taken the liberty to install @testing-library/react-hooks to support the tests, since it is a test dependency I think it will not affect production.

Copy link
Contributor

@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.

No issues regarding behaviour, good to go.
Did not experience the issue reported by Elena either.

Some minor nitpick comments.

@alfetopito
Copy link
Contributor

@elena-zh I have complied with points 1 and 2. Closes #700

  1. Changing the rows per page should reset position back to page 1
  2. When not on page 1, rows should not be added/removed, only update what's already visible. Now it causes

I tried point 3. but it didn't look very good, maybe @alongoni can help shape that idea.

No worries, let's iterate on it

@alfetopito @anxolin What do you think about adding later a dependency like react-table? it could help in the future when adding the other TradesTable and other filters.

No concerns if we see that's useful.
We didn't go for any lib at first since there was no need for it yet.

@alfetopito I have problems with the coverage of the useTable hook, but I see that the '@testing-library/react-hooks' library is not installed. Can we add it to cover the hook test?

No concerns either, I liked the tests 👍

@elena-zh
Copy link

elena-zh commented Oct 5, 2021

Changes LGTM now!
Thanks!

@alfetopito alfetopito added the Auto-merge PRs with this tag will be automatically merged when approved and CI succeeds label Oct 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
app:Explorer Explorer App Auto-merge PRs with this tag will be automatically merged when approved and CI succeeds Protofire task to the protofire team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a component to use as pager in user orders table
4 participants