-
Notifications
You must be signed in to change notification settings - Fork 58
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: ability to sort and filter orderbook #434
Conversation
05d1128
to
dc11a83
Compare
Let me know once this is ready to review. :) |
Ready for review! 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, looks great! ✨
Just a couple of smaller thigns, nothing major. Overall looks awesome -- good job!
src/components/Orderbook.tsx
Outdated
{search && ( | ||
<> | ||
<br />( | ||
{t('orderbook.text_orderbook_filter_count', { | ||
filterCount: tableData.nodes.length, | ||
})} | ||
) | ||
</> | ||
)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Screen.Recording.2022-08-04.at.10.18.31.mov
Adding that text below looks a bit chunky as the header size changes and the texts don't look aligned since one is in parentheses. What about just replacing the orderbook.text_orderbook_summary
text with the count of orders that's currently displayed? i.e.:
SEARCH: (none)
3 orders by 3 counterparties
ORDER: foobar
ORDER: foobaz
ORDER: bla
and
SEARCH: foo
2 orders by 2 counterparties
ORDER: foobar
ORDER: foobaz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
What do you think of adding a hint if the search is activated?
e.g. "2 orders by 2 counterparties (filtered)", "found 2 orders by 2 counterparties", "2 orders by 2 counterparties match given search criteria"...
Or do you think it is totally unnecessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to have "(filtered)" at the end, as I feel that some sort of visual feedback that the search value has actually been applied is a Good Thing™. However, if you think it is stupid and unncessary, I'll remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like "found 2 orders by 2 counterparties". It feels the most natural to read imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed the wording update myself, hope that's ok. :)
Co-authored-by: Daniel <[email protected]>
Co-authored-by: Daniel <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM ✅ ✨
Adds sorting, filtering and freshing functionality to the Orderbook component.
Keep in mind: This is still hidden behind a feature flag! It can be enabled for all users by merging #445
All "fullscreen-offcanvas" component have been generalized, that's why the style of the Earn Report component is affected by these changes.
📸
Empty orderbook (light mode)
Local orderbook (light mode)
Filtered orderbook (dark mode)
Earn report with harmonized styles (not using the new table library as of yet)