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

Permit filtering unassigned peers #425

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

EdouardVanbelle
Copy link
Contributor

@EdouardVanbelle EdouardVanbelle commented Nov 27, 2024

Please find a very small improvement in peers list:

  • permit a serial lookup on peers table
  • display the peer's serial number (to avoid. new column I added it in the OS section:
Serial Number
  • display of the serial number in the peer section

  • capability to filter peers that belong to the "All" group only, this is useful to track non assigned peers
    All group only

@EdouardVanbelle EdouardVanbelle force-pushed the add-serial-lookup branch 2 times, most recently from 4e56441 to 2c469c0 Compare November 29, 2024 21:55
@EdouardVanbelle EdouardVanbelle marked this pull request as ready for review November 29, 2024 22:01
@heisbrot
Copy link
Contributor

Hey @EdouardVanbelle,

thanks for this PR! I like the idea of filtering by unassigned peers.
However, we should look into having it in another position to keep the filters clean.

I was thinking of either in the groups filter or having a dropdown with (All Peers, Online Peers, Offline Peers, Unassigned Peers) instead of the current buttons. What do you think?

@EdouardVanbelle
Copy link
Contributor Author

EdouardVanbelle commented Dec 21, 2024

Hello @heisbrot interesting ! You are right the current case is a bit confusing

I am also thinking that All, Online, Offline filters complements the group filtering (you can mix both)

To complete your suggestion the dropdown with a selector before the separator and a checkbox on unassigned peers:

 ( ) All Peers
 ( ) Online Peers
 ( ) Offline Peers
 -----
 [ ] unassigned peers

I find more coherent your first suggestion, meaning keeping All, Online, Offline buttons and complete the dropdown group like this:

  [ ] groupA 
  [ ] groupB
  [ ] groupC
  -----
  [ ] unassigned peers

Right now I have this incoherent case: (I must implement an exclusivity)

Capture d’écran 2024-12-21 à 00 57 41

@EdouardVanbelle
Copy link
Contributor Author

Hello @heisbrot , I applied your suggestions, which brings:

Capture d’écran 2024-12-22 à 23 58 31 Capture d’écran 2024-12-22 à 23 58 46

Changes:

  • there is an exclusivity between Unassigned peers and other group selection to keep consistency on this selector
  • breaking change: I have also removed the All group in the drop down as all peers are always in this group, this part is arguable, up to you :)
  • the Group filter is hidden if the only one group All exists, arguable too :)
  • I have added a function overrideTableFilter() in order to keep other columns selection and reduce code redundancy. I guess there is a smarter solution but I don't know this framework
  • breaking change: take care that now the Pending Approvals button keeps other column filters (use of overrideTableFilter() function)
  • breaking change: I have also swapped buttons' order to keep filters together, and the RowsPerPage later, still up to you :)
  • the serial is still present in this merge request

🙏 all my appologies, this is the first time I code in React

@EdouardVanbelle EdouardVanbelle force-pushed the add-serial-lookup branch 3 times, most recently from 960200f to 9d41021 Compare December 23, 2024 00:38
@EdouardVanbelle EdouardVanbelle force-pushed the add-serial-lookup branch 2 times, most recently from 1c97224 to 1a6a245 Compare January 29, 2025 11:33
@EdouardVanbelle
Copy link
Contributor Author

Hello @heisbrot I have rebased these commit to resolve a conflict
let me know if you prefer I split this merge request in 2:
1 for serial
1 for the filtering capability on unassigned peers

Regards

@heisbrot
Copy link
Contributor

Hello @heisbrot I have rebased these commit to resolve a conflict let me know if you prefer I split this merge request in 2: 1 for serial 1 for the filtering capability on unassigned peers

Regards

Hey @EdouardVanbelle ,

thank you! Yes a split would be great. We can then directly merge the serial.
The filtering stuff still needs review and some testing from my end. But overall the changes make sense :)

@EdouardVanbelle EdouardVanbelle changed the title Add serial lookup on peers table + permit filtering on 'All' group only Permit filtering unassigned peers Jan 29, 2025
@EdouardVanbelle
Copy link
Contributor Author

Here it is @heisbrot

you can review first the pull request #444 to validate the serial feature
(I did not squashed your patch)

Then this pull request will contain only the unassigned peers filter

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