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: add sorting and filtering to earn report #451

Merged
11 commits merged into from
Aug 5, 2022
Merged

Conversation

theborakompanioni
Copy link
Collaborator

@theborakompanioni theborakompanioni commented Aug 4, 2022

Adds pagination, sorting and filtering to Earnings Report overlay.

Was not quite sure how to best style the pagination. Not that happy with the result. Suggestions welcome!
Also, any ideas for improved wording of the table headings?

📸

image

@theborakompanioni theborakompanioni self-assigned this Aug 4, 2022
@theborakompanioni theborakompanioni added the UI/UX Issue related to cosmetics, design, or user experience label Aug 4, 2022
@theborakompanioni theborakompanioni requested a review from a user August 5, 2022 07:37
@theborakompanioni theborakompanioni marked this pull request as ready for review August 5, 2022 07:37
@ghost
Copy link

ghost commented Aug 5, 2022

Was not quite sure how to best style the pagination. Not that happy with the result. Suggestions welcome!

We can also omit it for now if you're not happy with it. I feel like it's not super essential to have.

Copy link
Contributor

@MaxHillebrand MaxHillebrand left a comment

Choose a reason for hiding this comment

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

How about "my inputs" or "my amounts" as table header?

Maybe for pagination a simple "load more" button at the bottom of the table? Seems like less ui clutter.

@ghost
Copy link

ghost commented Aug 5, 2022

Pagination is nice (maybe needs a bit of a cleaner UI to feel less cluttered) but I feel like it'd be more interesting for the orderbook than for the earn report since the orderbook will be hundreds of entries long by default.

@theborakompanioni
Copy link
Collaborator Author

Pagination is nice (maybe needs a bit of a cleaner UI to feel less cluttered) but I feel like it'd be more interesting for the orderbook than for the earn report since the orderbook will be hundreds of entries long by default.

Very valid point. Any immediate ideas or recommendations on the styling? Or should it be added and adapted in an upcoming version?

@ghost
Copy link

ghost commented Aug 5, 2022

The easiest would probably be to come up with a UI and add it in one go to both tables? 🤔

src/components/EarnReport.tsx Outdated Show resolved Hide resolved
@theborakompanioni
Copy link
Collaborator Author

I'll be removing the pagination for now - it's not the best in terms of UI/UX. Will be readding it in a new PR to the Earn Report and Orderbook components.

@theborakompanioni theborakompanioni changed the title feat: add pagination, sorting and filtering to earn report feat: add sorting and filtering to earn report Aug 5, 2022
const entry = toEarnReportEntry(item)
return (
<Row key={item.id} item={item}>
<Cell>{entry.timestamp.toLocaleString()}</Cell>
Copy link

@ghost ghost Aug 5, 2022

Choose a reason for hiding this comment

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

I personally prefer ISO strings for timestamps. Might be a personal thing though. Any thoughts? Not sure what we use in other places throughout the app. Might be that we're a bit inconsistent there.

@theborakompanioni
Copy link
Collaborator Author

  • Pagination removed ✅
  • Tests for parsing data added ✅

ping @dnlggr

@ghost ghost dismissed MaxHillebrand’s stale review August 5, 2022 11:32

Column headers were updated.

@ghost ghost merged commit bd8fd97 into master Aug 5, 2022
@ghost ghost deleted the earn-report-pagination branch August 5, 2022 11:56
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
UI/UX Issue related to cosmetics, design, or user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants