Skip to content

Conversation

amovfx
Copy link
Contributor

@amovfx amovfx commented Feb 8, 2023

Description

Updated Network card to display the amount of taro nodes.

Steps to Test

  1. Create a new network with taro nodes.
  2. Go to network selection panel.

Copy link
Contributor Author

@amovfx amovfx left a comment

Choose a reason for hiding this comment

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

Reviewed with tests and ci passing.

Copy link
Owner

@jamaljsr jamaljsr left a comment

Choose a reason for hiding this comment

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

Great idea! I didn't even think to update these. Thank you.

Just have a few minor suggestions.

@@ -43,6 +43,13 @@ const NetworkCard: React.FC<{ network: Network }> = ({ network }) => {
suffix={<LinkOutlined />}
/>
</Col>
<Col span={12}>
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not a big fan of the empty space in the second column. You could get all 3 on one row by setting span={8} on all columns and removing the "Nodes" from the titles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

<Statistic
title={l('taroNodes')}
value={network.nodes.taro.length}
suffix={<ApiOutlined />}
Copy link
Owner

Choose a reason for hiding this comment

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

antd doesn't provide a lot of icons to choose from but I think the DollarOutlined one makes more sense for Taro since the primary goal of the project is to bring stablecoins to Bitcoin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Owner

@jamaljsr jamaljsr left a comment

Choose a reason for hiding this comment

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

This looks good to go. Going to merge. Thanks for the update

@jamaljsr jamaljsr merged commit 908a0bd into jamaljsr:feat/add-taro Feb 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants