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

frontend: RecentClusters: Add multi cluster selection feature #2513

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

Conversation

illume
Copy link
Collaborator

@illume illume commented Nov 3, 2024

If there are multiple clusters then this allows selecting up to three of them from the recent clusters list.

If there is a single cluster it acts as before.

This feature can be enabled by using:

  • REACT_APP_MULTI_ENABLED=true make run-frontend

Screenshot

Here we see a toggle button group of the three most recently used clusters.

image

As a bonus this fixes a bug:

  • text is not cut off (important when the shared resource/subscription name is the start of the cluster name)

How to test

  • have at least two clusters enabled, and select the clusters you want to use at once, it takes you to the correct URL with a + separating the clusters
  • when you don't select any clusters it doesn't let you select the view button

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Nov 3, 2024
@illume illume force-pushed the RecentClusters-multi branch from 9462687 to 4b35fd4 Compare November 3, 2024 14:43
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Nov 3, 2024
@illume illume added enhancement New feature or request frontend Issues related to the frontend multi Multi cluster aggregated view bug Something isn't working labels Nov 3, 2024
If there are multiple clusters then this allows selecting up
to three of them from the recent clusters list.

If there is a single cluster it acts as before.

This feature can be enabled by using:
- `REACT_APP_MULTI_ENABLED=true make run-frontend`

Signed-off-by: René Dudfield <[email protected]>
@illume illume requested a review from a team November 7, 2024 16:34
@joaquimrocha
Copy link
Collaborator

This should be replaced by the commits in https://github.com/headlamp-k8s/headlamp/commits/aggregated-view-original-rebased/ (lookout for fixups) related to e9b7f46 .

@sniok
Copy link
Contributor

sniok commented Nov 8, 2024

The button group showing recent clusters doesn't seem to be very intuitive, I think it would be hard to understand
To be honest I personally never use recent cluster buttons, I always select from the list

What if we did something like this? (names would still be links, just not shown on the screenshot)

image

@illume
Copy link
Collaborator Author

illume commented Nov 8, 2024

Yeah, we can add multiple select with checkboxes like that. But I'll explain why I didn't... and then talk about the checkboxes below.

The button group showing recent clusters doesn't seem to be very intuitive, I think it would be hard to understand
To be honest I personally never use recent cluster buttons, I always select from the list

For what it's worth I tried it with two people without prompting "Select multiple clusters" and they figured it out. The third person I just showed the video and they suggested changing it to use the existing Recent cluster cards instead of just the words.

Mainly this code is a temporary step to allow something now and until both

  1. the full 'new home' design is implemented and can be iterated on/user tested
  2. and the websocket multiplexer is integrated and tested.

The reasons are:

  • The idea is to limit it to 3 clusters until the multiplexer is in properly
  • It lets people test the rest of the multi cluster more easily without waiting for the rest of the stuff
  • We can use it in a demo in Kubecon in a few days
  • This can be enabled/disabled trivially

checkboxes version

What about these changes to your "selected checkboxes" image above in a temporary step?

  • Your message but changed slightly to "Selected 2 of 3"
  • Maximum of 3 selected, and then it won't let you add any more, with an additional message. "Selected 3 of 3. Currently only three clusters can be selected at once"

(I would keep the changes in Recent clusters except change them to use the existing card design as requested)

@illume
Copy link
Collaborator Author

illume commented Nov 8, 2024

This should be replaced by the commits in https://github.com/headlamp-k8s/headlamp/commits/aggregated-view-original-rebased/ (lookout for fixups) related to e9b7f46 .

@joaquimrocha we discussed this is a temporary measure for reasons mentioned here. Additionally it was discussed and agreed to take this approach with Chris. First a minimal version that is disabled, and then the full version.

@sniok
Copy link
Contributor

sniok commented Nov 8, 2024

If this is temporary and was user tested a bit then it's okay, we can do checkboxes later

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Nov 8, 2024
@illume
Copy link
Collaborator Author

illume commented Nov 8, 2024

@sniok ok

For the checkboxes is what I proposed to handle maxium of 3 sound ok? Or if you have any other ideas for that I'd appreciate it.

@sniok
Copy link
Contributor

sniok commented Nov 8, 2024

For the checkboxes is what I proposed to handle maxium of 3 sound ok? Or if you have any other ideas for that I'd appreciate it.

Yeah that's reasonable. Although I'm a bit worried about allowed namespaces feature. If we have 2 allowed namespaces for each of 3 selected clusters and go to Workloads page then we'll get (7 workloads)(3 cluster)(2 namespaces) websockets. But it's and edgecase.

Maximum of 3 selected, and then it won't let you add any more, with an additional message. "Selected 3 of 3. Currently only three clusters can be selected at once"

I think it's better to disable the "connect" (or "view") button (keep the message, but highlight in red "Selected 4 of 3. Currently only...") but still allow selecting checkboxes, I think it'll be less frustrating

@illume
Copy link
Collaborator Author

illume commented Nov 8, 2024

Thanks @sniok

@farodin91
Copy link
Contributor

@illume i started to work on multi delete feature. You table and my table changes should be merged. What do you think of my design by hiding filter during the selection? #2594

@joaquimrocha
Copy link
Collaborator

ATM we should rather add the named groups feature instead of a radio button based solution.
We should still keep the multiple selection of clusters so we create a group (named) from them.

@illume illume added this to the v0.29.0 milestone Jan 28, 2025
});

/** Allow selecting multiple recent clusters. */
const MULTI_ENABLED = import.meta.env.REACT_APP_MULTI_ENABLED === 'true' || true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am willing to merge this PR as a temporary way to test this, but seems like this is enabled all the time.

@illume
Copy link
Collaborator Author

illume commented Feb 18, 2025

  • to be prepared to be merged at the start of the next release cycle
  • change to checkboxes to view, not the current toggle box thing
  • leave on by default, and then before the release it can be disabled if it's not working
  • This will let us devs test during the release cycle

@joaquimrocha joaquimrocha modified the milestones: v0.29.0, v0.30.0 Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request frontend Issues related to the frontend lgtm This PR has been approved by a maintainer multi Multi cluster aggregated view size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
Development

Successfully merging this pull request may close these issues.

4 participants