-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add support for filtering latest TX list for method type #1679
base: master
Are you sure you want to change the base?
Conversation
Deployed to Cloudflare Pages
|
051268e
to
07e7251
Compare
0b7e0ce
to
4a589e6
Compare
6befb60
to
e092000
Compare
As discussed with @donouwens, the type filter list for the consensus layer is too long to be convenient. Ideally, it should be replaced with a combo-box with type-ahead search. However, we plan to support multiple values anyway (later, when nexus will add support for that), so we will replace the widget with a searchable combo-box at that time. Which means, the current version is as good as it will get, in the current round of implementation. |
// Please note: when updating this, keep it in sync | ||
// with the knownConsensusTxMethods array below! | ||
|
||
switch (method) { |
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.
Wouldn't this be better if there was an object map, having O(1) time complexity looking up each of the elements.
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.
Maybe. We have the exact same system for Runtime transactions, here, introduced ages ago. If we want to refactor this, it should be done consistently, on both ends.
However it's not entirely trivial because we need to rely on a t
function, which is not available by default in the module namespace, so we can only prepare the object map when the first call happens. Then maybe we can try to cache and reuse it, but I feel that this might be premature optimization and opens the possibility of race conditions at the time of first rendering... not sure it's worth the effort.
What do you think?
src/app/components/Select/index.tsx
Outdated
@@ -96,6 +97,54 @@ const TertiaryButton = forwardRef( | |||
}, | |||
) | |||
|
|||
export const StyledLightSelectButton = styled(StyledSelectButton)(() => ({ |
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.
Not a fan of this, creating new components, rather I would add some kind of prop to StyledSelectButton
, that would change the styling based on that via slotProps
.
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 did this now, but I am not sure if that made the code simpler or not.
src/app/components/Select/index.tsx
Outdated
}, | ||
})) | ||
|
||
const LightTertiaryButton = forwardRef( |
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.
This duplication seems unnecessary, I would rather see css changes based on props.
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 did this now, but I am not sure if that made the code simpler or not.
/** | ||
* An optional second title which will be displayed under title / subheader / action | ||
*/ | ||
title2?: ReactNode |
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.
subtitle
maybe?
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.
The problem is that currently we have title
, subheader
(and action
), all displayed in the same row.
If we introduced subtitle
, it would be very easy to accidentally confuse it with subheader
.
That was my thinking, but I don't have a strong opinion. If you think subtitle would be better, I am open to rename it. What do you think?
@@ -166,6 +175,7 @@ export const Table: FC<TableProps> = ({ | |||
<TablePagination {...pagination} /> | |||
</Box> | |||
)} | |||
{!isLoading && !rows?.length && emptyMessage && <CardEmptyState label={emptyMessage} />} |
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.
This seems like it would render the <table />
and then the empty message bellow 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.
The table header, and the empty message, yes. Like this:
I find this more useful than hiding the whole table.
In any case, this was required in the initial version, when the filter widget for the TX type was placed in the table column header for type. In that arrangement, it was absolutely necessary that the table header stays, even if there is no data, so that we can change the filter to get data. But we have changed the design, so now we could live with a disappearing table. Would you prefer that?
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.
Typescript can make sure these are kept in sync
Currently, if a <Table> receives zero rows, and it is not loading, it is simply hidden. This is not ideal if there are zero rows because of some filtering configured inside the table headers, which is now no longer accessible, since the whole table has been hidden. So this commit introduces an alternative behavior: if we pass an `emptyMessage` to the table, then it will not be hidden even if there are no rows, but instead, the passed message will be displayed. Please note that the default behavior doesn't change; to trigger this new behavior, the new parameter needs to be added.
ca33523
to
e6dcb06
Compare
Nexus has introduced filtering for tx method in oasisprotocol/nexus#824 Our API wrapper code hasn't yet been updated for this change. This commit selectively updates this single part of the wrapper.
This works on: - the latest transactions page (i.e. /mainnet/sapphire/tx ) - the account transactions page ( /mainnet/sapphire/address/0x683dC5f8cFa3e156b0F695CEa0b8EdeC7322CbF6 )
(As preparation for filtering for them)
- Add missing methods - Use exhaustedTypeWarning for warning on new, unhandled values Co-authored-by: Luka Jeran <[email protected]> Add exhaustedTypeWarning for consensus method label generation
e6dcb06
to
0b41a3c
Compare
This PR exposes the method search feature added to Nexus in oasisprotocol/nexus#824.
It adds support for filtering latest TX list for method type.
This now works on all consensus and runtime transaction list, also including transactions for blocks, accounts and validators.
I suggest reviewing the PR commit by commit, because there are multiple semi-independent small changes.
The account transactions page looks like this:
Or this, when there are no results:
Latest transactions page, on mobile:
For consensus: