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

[UI] Nodes sorting #2926

Merged
merged 12 commits into from
Nov 19, 2020
Merged

[UI] Nodes sorting #2926

merged 12 commits into from
Nov 19, 2020

Conversation

alexis-ld
Copy link
Contributor

@alexis-ld alexis-ld commented Nov 13, 2020

Component:
ui, nodes

Context:
We want to be able to sort the Nodes table like the Volumes table.

Summary:
I used the same sorting logic (leveraging react-table's sorting abilities).
I extracted the URL sync code to its custom hook and the Table sorting components to the Common layout file to avoid code redundancy between the two views.
Update:

  • Introduce hook test libray
  • Bump React version to the latest v17.0.1

Acceptance criteria:
Clicking on a column header on the Nodes table sorts the data according to what is specified in #2919 .
Loading the page with a sorting specified in the URL sorts the data accordingly.


Closes: #2919

Alex Le Dinh added 9 commits November 12, 2020 15:53
Wait until alerts and PVClist are retrieved before auto-selecting first volume based on health.

Refs: #2920
Remove default sorting from query string on volumes list

Refs: #2920
Use a flag on the whole volumes instead of on each item to determine when alerts have been mapped

Refs: #2920
Extract Sorting styled components to re-use those in Nodes table

Refs: #2919
Base for Nodes sorting, added react-table utilities and basic header triggers

Refs: #2919
Add missing constant for Node status

User constants instead of strings in NodeUtils table data generator to be consistent with Sorting

Implement Status Sorting

Refs: #2919
Add name lowercase sorting

Refs: #2919
Move the URL Sync logic for both Volumes and Nodes tables to its custom hook function to avoid a big block of code redundancy.

Refs: #2919
Add missing history dependency in useEffect in Sort URL Sync custom hook

Refs: #2919
@alexis-ld alexis-ld requested a review from a team as a code owner November 13, 2020 10:40
@bert-e
Copy link
Contributor

bert-e commented Nov 13, 2020

Hello alexis-ld,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Status report is not available.

@bert-e
Copy link
Contributor

bert-e commented Nov 13, 2020

Integration data created

I have created the integration data for the additional destination branches.

The following branches will NOT be impacted:

  • development/1.0
  • development/1.1
  • development/1.2
  • development/1.3
  • development/2.0
  • development/2.1
  • development/2.2
  • development/2.3
  • development/2.4
  • development/2.5

You can set option create_pull_requests if you need me to create
integration pull requests in addition to integration branches, with:

@bert-e create_pull_requests

@bert-e
Copy link
Contributor

bert-e commented Nov 13, 2020

Waiting for approval

The following approvals are needed before I can proceed with the merge:

  • the author

  • one peer

Peer approvals must include at least 1 approval from the following list:

thomasdanan
thomasdanan previously approved these changes Nov 13, 2020
Copy link
Contributor

@thomasdanan thomasdanan left a comment

Choose a reason for hiding this comment

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

works well for me ...

Copy link
Contributor

@ChengYanJin ChengYanJin left a comment

Choose a reason for hiding this comment

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

Smart move of useTableSortURLSync hook! Bravo! I suggest we add a unit test for this.

BTW, since by default we sort the node list by health. Shouldn't we auto-select the first node base on the health?

Thanks!

@bert-e
Copy link
Contributor

bert-e commented Nov 14, 2020

Waiting for approval

The following approvals are needed before I can proceed with the merge:

  • the author

  • one peer

Peer approvals must include at least 1 approval from the following list:

The following reviewers are expecting changes from the author, or must review again:

Alex Le Dinh added 2 commits November 17, 2020 11:34
Version >= 16.9 needed in order to use the custom hook testing library

Refs: #2919
Add react-hooks-testing-library to render hooks within a test component

Test useTableSortURLSync to check if URL params are handled properly

Refs: #2919
@alexis-ld
Copy link
Contributor Author

@ChengYanJin I added tests for the custom hook as well as the Node auto-select

Copy link
Contributor

@ChengYanJin ChengYanJin left a comment

Choose a reason for hiding this comment

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

LGTM!
Thanks for introducing the hook test library and bump React to the latest version ;)

@bert-e
Copy link
Contributor

bert-e commented Nov 18, 2020

Waiting for approval

The following approvals are needed before I can proceed with the merge:

  • the author

  • one peer

Peer approvals must include at least 1 approval from the following list:

Copy link
Contributor

@thomasdanan thomasdanan left a comment

Choose a reason for hiding this comment

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

works as expected

@alexis-ld
Copy link
Contributor Author

/approve

@bert-e
Copy link
Contributor

bert-e commented Nov 18, 2020

In the queue

The changeset has received all authorizations and has been added to the
relevant queue(s). The queue(s) will be merged in the target development
branch(es) as soon as builds have passed.

The changeset will be merged in:

  • ✔️ development/2.6

  • ✔️ development/2.7

The following branches will NOT be impacted:

  • development/1.0
  • development/1.1
  • development/1.2
  • development/1.3
  • development/2.0
  • development/2.1
  • development/2.2
  • development/2.3
  • development/2.4
  • development/2.5

There is no action required on your side. You will be notified here once
the changeset has been merged. In the unlikely event that the changeset
fails permanently on the queue, a member of the admin team will
contact you to help resolve the matter.

IMPORTANT

Please do not attempt to modify this pull request.

  • Any commit you add on the source branch will trigger a new cycle after the
    current queue is merged.
  • Any commit you add on one of the integration branches will be lost.

If you need this pull request to be removed from the queue, please contact a
member of the admin team now.

The following options are set: approve

@bert-e
Copy link
Contributor

bert-e commented Nov 19, 2020

I have successfully merged the changeset of this pull request
into targetted development branches:

  • ✔️ development/2.6

  • ✔️ development/2.7

The following branches have NOT changed:

  • development/1.0
  • development/1.1
  • development/1.2
  • development/1.3
  • development/2.0
  • development/2.1
  • development/2.2
  • development/2.3
  • development/2.4
  • development/2.5

Please check the status of the associated issue None.

Goodbye alexis-ld.

@bert-e bert-e merged commit 7453d9c into development/2.6 Nov 19, 2020
@bert-e bert-e deleted the improvement/2919-nodes-sorting branch November 19, 2020 08:03
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.

Ability to sort in MetalK8s UI Nodes table
4 participants