-
Notifications
You must be signed in to change notification settings - Fork 588
HDDS-3798. Collect more info about DN in recon web #1136
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
Conversation
|
@runitao That's wonderful, but you should fix the checkstyle issue first. |
|
@runitao Thanks for the patch! Can we make these two columns optional? We can have a "show/hide columns" button to have a multi-select dropdown and make these two columns not display by default. The users can then choose which columns to show/hide. I don't like adding lots of columns without an option to hide some of them to give more space to most used columns. |
|
Thanks @vivekratnavel for review, I will add the button. |
|
@runitao hope to see this feature in the recent release. |
|
@vivekratnavel PTAL, Thanks |
vivekratnavel
left a comment
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.
Overall LGTM. I have left a couple of inline comments.
I suspect some lint errors that should be fixed. Can you please run yarn run lint and fix any errors? Running yarn run lint:fix should fix errors automatically. Thanks!
| dataSource, | ||
| totalCount, | ||
| lastUpdated: Number(moment()) | ||
| lastUpdated: Number(moment()), |
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.
nit: dangling comma
| lastUpdated: Number(moment()), | |
| lastUpdated: Number(moment()) |
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.
fixed
| <Table | ||
| dataSource={dataSource} | ||
| columns={COLUMNS.filter((column) => | ||
| selectedColumns.some((e) => e.label === column.key) |
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.
nit: compare value instead of label
| selectedColumns.some((e) => e.label === column.key) | |
| selectedColumns.some((e) => e.value === column.key) |
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.
fixed
|
Thanks @vivekratnavel for review. The |
vivekratnavel
left a comment
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.
+1 LGTM
I think this failure case is unrelated with my PR. Please help me trigger CI again |
|
@avijayanhwx Would you please help me review this PR? |
|
@runitao Thanks for working on this! I have merged the change. |
…pache#1136)" This reverts commit f566187.
What changes were proposed in this pull request?
Add datanode version and setup time in recon web.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-3798
How was this patch tested?
update on 2020/07/05
As @vivekratnavel suggestion, I have add a multi-select dropdown to hide/show some columns. Here is the effect