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

670 component order limiter #686

Merged
merged 2 commits into from
Sep 28, 2021
Merged

670 component order limiter #686

merged 2 commits into from
Sep 28, 2021

Conversation

henrypalacios
Copy link
Contributor

@henrypalacios henrypalacios commented Sep 23, 2021

Summary

Closes #670

  • Create a reusable component, and a storybook of it.
  • Use a component in top tab header.
  • Pass limit filter to table when selecting a value.

To Test

  1. Go to https://pr686--gpui.review.gnosisdev.com/rinkeby/address/0xb6BAd41ae76A11D10f7b0E664C5007b908bC77C9/
  2. Storybook Dropdown componenteUI

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

@henrypalacios henrypalacios changed the base branch from develop to new-api-orderstable September 23, 2021 17:59
@henrypalacios henrypalacios force-pushed the 670-component-order-limiter branch from 16e0f90 to d3fae04 Compare September 24, 2021 15:38
@henrypalacios henrypalacios marked this pull request as ready for review September 24, 2021 20:39
@alongoni alongoni self-requested a review September 24, 2021 20:42
@henrypalacios henrypalacios force-pushed the 670-component-order-limiter branch from e539bc7 to e6efdb5 Compare September 24, 2021 21:26
@henrypalacios
Copy link
Contributor Author

@alfetopito
The Dropdown component could go in src/comonents/common/Dropdown, WDYT?

What do you think about these options [10, 20, 30, 50] in the paging selector?

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.

I think you went a bit overboard here and added stuff we'll probably not need, but good to have anyway :)

About the storybook, when I pick something from the dropdown the selection doesn't change... 🤷 not a big deal if takes too long to fix

About the real table, add some padding/margin on the right. The dropdown glued to the border
screenshot_2021-09-24_15-06-41

Some minor comments/questions below.

Great job!

Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

Not necessarily for this PR, but if we are adding this control, should we add it also to the query params when you change the default page size?

@henrypalacios
Copy link
Contributor Author

@anxolin You mean if I had the limiter set to 20 and I change the screen size we should adjust the limiter pageSize selected according to the new size of the screen.

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 great!

Nice to have / UX improvement:
When the arrows are implemented, we should duplicate the component at the end of the order list.

@alfetopito
Copy link
Contributor

@anxolin You mean if I had the limiter set to 20 and I change the screen size we should adjust the limiter pageSize selected according to the new size of the screen.

Not quite. I believe he meant to update the URL with the preferred page size, in case for example if we reflect user's selection in the URL, so it's shareable: .../address/0x..../?pageSize=20

But as Anxo said, not for now.
We can keep this in mind for later.

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.

LGTM, only minor comments

Base automatically changed from new-api-orderstable to develop September 27, 2021 18:09
@henrypalacios henrypalacios force-pushed the 670-component-order-limiter branch from 74047e8 to e9c59b0 Compare September 27, 2021 21:27
Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

Not quite. I believe he meant to update the URL with the preferred page size, in case for example if we reflect user's > selection in the URL, so it's shareable: .../address/0x..../?pageSize=20
But as Anxo said, not for now.
We can keep this in mind for later.

Yep, exactly as Leandro said. All good. Thanks

@alfetopito alfetopito added the Auto-merge PRs with this tag will be automatically merged when approved and CI succeeds label Sep 28, 2021
@mergify mergify bot merged commit b30d48b into develop Sep 28, 2021
@alfetopito alfetopito deleted the 670-component-order-limiter branch September 28, 2021 15:12
@alfetopito alfetopito mentioned this pull request Oct 13, 2021
@henrypalacios henrypalacios mentioned this pull request Feb 1, 2022
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 an order limiter
4 participants