frontend: allow multiple deletes on ResourceTable #2594

farodin91 · 2024-11-21T15:55:47Z

Fixes #1640, #1604

allow rowselection
externalize logic

screenshots:

folllow up:

support more actions such as restarts
support auth check
support plugin extensions

skoeva · 2024-11-21T17:25:05Z

Thanks! minor nit, can you change the following file name:

ResourecTableMultiActions.tsx
⬇️
ResourceTableMultiActions.tsx

sniok

Great improvement, thanks! I've tried it out and works well. Just left a couple of small suggestions

frontend/src/components/common/ConfirmDialog.tsx

frontend/src/components/common/Table/Table.tsx

frontend/src/components/common/Resource/DeleteMultipleButton.tsx

farodin91 · 2024-11-22T15:11:40Z

I found an issue. It should be fixed with the next commit.

joaquimrocha · 2024-11-26T11:50:53Z

My concern here is how do we employ our usual UI philosophy of not showing the controls for which the user has no permissions. i.e. how to make sure we don't show the delete button if we are in a role where a deletion will fail? (some users deploy Headlamp as a read-only UI)
Should we make an exception here and still allow users to select multiple rows but then check if there are no permissions to delete those items and, if no items can be deleted, we either 1. place a disabled button with a tooltip explaining there's no permissions; 2. we show a message right away in the actions area explaining there's no permissions?

farodin91 · 2024-11-26T16:47:30Z

@joaquimrocha Is their an efficient way to check all items for permission?

farodin91 · 2024-11-26T16:49:51Z

We could also show in the dialog for which resources you have the permission as you can have the permission for some and for some you have no permission.

farodin91 · 2024-11-26T16:53:08Z

I did thought a bit about it during the implementation, but I think their is no perfect way. So, I left it away.

First I could a global setting to show it or not and then mark it as beta and ask for feedback.

joaquimrocha · 2024-11-26T17:49:00Z

I did thought a bit about it during the implementation, but I think their is no perfect way. So, I left it away.

First I could a global setting to show it or not and then mark it as beta and ask for feedback.

I think we can ask on Slack. But it's true that there's no easy way to do this very efficiently, because theoretically I guess one could be allowed to delete just a single resource in a namespace where nothing else can be deleted in one's role, right?

I think we could also implement your idea of a switch in settings as you say, but I guess the idea of not showing the delete button is only relevant in environments where Headlamp is read-only most of the time, and those tend to be when Headlamp is deployed in-cluster or as a service. So what would be cool here is a way for running Headlamp with pre-filled settings, but we don't have a good solution for this ATM.

I would say it's more important to have bulk deletion than a way to avoid it in RO envs, so we can keep your solution if there's no way or half way to check for permissions in bulk...

farodin91 · 2024-11-26T18:11:22Z

I think we can ask on Slack.
Sounds great.

But it's true that there's no easy way to do this very efficiently, because theoretically I guess one could be allowed to delete just a single resource in a namespace where nothing else can be deleted in one's role, right?
yeah, also one namespace is allowed another is prohibited.

For the switch: Do you think it is better to use a single switch or a switch for every cluster?

joaquimrocha · 2024-11-27T09:54:37Z

For the switch: Do you think it is better to use a single switch or a switch for every cluster?

It would be a switch for every cluster, but I don't think we should add the switch for now, as it won't make much difference since it's the end user themselves, not an admin, that would have to set the preference. And there's no big gain in that, since our concern here is to have people eventually trying to delete resources in a Headlamp that's been deployed for them, not in their own desktop deployment.

So for now we can leave that functionality as an exception to our overall UI controls philosophy IMO.

farodin91 · 2024-11-27T16:28:13Z

@joaquimrocha What is your plan now? Keep the current state and probably add a beta flag to it. Or do we need to do more merging?

farodin91 · 2024-11-27T21:06:57Z

@joaquimrocha I added a beta mark to deleting items

joaquimrocha · 2024-11-28T09:55:54Z

@joaquimrocha What is your plan now? Keep the current state and probably add a beta flag to it. Or do we need to do more merging?

Sorry I couldn't reply earlier, and that I wasn't clearer in my previous message.

In the meanwhile, I have a few more points to discuss:
I was wondering if we should enable clicking the row to select, and only show the check boxes column if one row is selected, but maybe this hides the feature too much? I can talk to our design team next Wednesday and get their feedback too.

I don't think a beta flag is needed, but I wonder if we should have a REACT_APP*_ flag to enable it (we use this to change some things, like the product name). This would enable/disable the feature be for all clusters, so it could give the ability for someone deploying a read-only Headlamp to turn this feature off.

Some changes needed already:
After I tested it now, I notice that there's one notification per pods. This is a problem, since it will not only look noisy but also it will be impossible for the user to click cancel in more than a few. So the delete actions should be gathered in one action "Deleting items my-pods-123, my-pods-321, ... [Cancel]". We have a similar case when we apply multiple resources from the create dialog.

farodin91 · 2024-11-30T12:15:27Z

@joaquimrocha I've grouped the deletion. "Event shows a number"
I added a flag.

I was wondering if we should enable clicking the row to select, and only show the check boxes column if one row is selected, but maybe this hides the feature too much? I can talk to our design team next Wednesday and get their feedback too.

I just used the design which react table is providing.

joaquimrocha

Left a couple of comments but overall is looking good.
Also, the UX feels very slow. Takes sometimes 2 seconds for an item to be selected when in dev mode. Dev mode is admittedly very slow, but maybe @sniok knows whether there's something we can do to speed this up.

frontend/src/components/common/Resource/ResourceTable.tsx

frontend/src/components/common/ConfirmDialog.tsx

frontend/src/components/common/Resource/DeleteMultipleButton.tsx

joaquimrocha · 2024-12-02T17:46:37Z

frontend/src/components/common/Resource/DeleteMultipleButton.tsx

+            clonedItems.forEach(item => {
+              item.delete();
+            });


I wonder if we could run these in a promise and wait for all.

I will check it out.

What do you think?

Signed-off-by: farodin91 <[email protected]>

farodin91 · 2024-12-02T22:21:14Z

@joaquimrocha i merged the branch table-pref into this branch and it was quite fast.

* allow row selection * externalize logic Signed-off-by: farodin91 <[email protected]>

farodin91 force-pushed the row-selection branch from d3e5a94 to e150cb4 Compare November 21, 2024 15:59

farodin91 marked this pull request as ready for review November 21, 2024 16:10

dosubot bot added the size:XXL This PR changes 1000+ lines, ignoring generated files. label Nov 21, 2024

farodin91 force-pushed the row-selection branch 3 times, most recently from 09e1b10 to 77bd64d Compare November 22, 2024 09:31

sniok requested changes Nov 22, 2024

View reviewed changes

frontend/src/components/common/ConfirmDialog.tsx Outdated Show resolved Hide resolved

frontend/src/components/common/Table/Table.tsx Outdated Show resolved Hide resolved

frontend/src/components/common/Resource/DeleteMultipleButton.tsx Outdated Show resolved Hide resolved

farodin91 force-pushed the row-selection branch from abddc30 to dbd5e96 Compare November 22, 2024 15:38

joaquimrocha self-requested a review November 26, 2024 10:25

farodin91 mentioned this pull request Nov 26, 2024

frontend: RecentClusters: Add multi cluster selection feature #2513

Open

2 tasks

farodin91 force-pushed the row-selection branch from dbd5e96 to 26f6337 Compare November 27, 2024 21:05

farodin91 force-pushed the row-selection branch 2 times, most recently from e148634 to b91cd2a Compare November 30, 2024 12:12

joaquimrocha requested changes Dec 2, 2024

View reviewed changes

frontend: Allow ReactNode as description for ConfirmDialogs

4ddc587

Signed-off-by: farodin91 <[email protected]>

farodin91 force-pushed the row-selection branch from b91cd2a to 5fac47e Compare December 2, 2024 22:16

frontend: Allow multiple deletes on ResourceTable

bff6644

* allow row selection * externalize logic Signed-off-by: farodin91 <[email protected]>

farodin91 force-pushed the row-selection branch from 5fac47e to bff6644 Compare December 2, 2024 22:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

frontend: allow multiple deletes on ResourceTable #2594

frontend: allow multiple deletes on ResourceTable #2594

farodin91 commented Nov 21, 2024 •

edited

Loading

skoeva commented Nov 21, 2024

sniok left a comment

farodin91 commented Nov 22, 2024

joaquimrocha commented Nov 26, 2024

farodin91 commented Nov 26, 2024 •

edited

Loading

farodin91 commented Nov 26, 2024

farodin91 commented Nov 26, 2024

joaquimrocha commented Nov 26, 2024

farodin91 commented Nov 26, 2024

joaquimrocha commented Nov 27, 2024

farodin91 commented Nov 27, 2024

farodin91 commented Nov 27, 2024

joaquimrocha commented Nov 28, 2024

farodin91 commented Nov 30, 2024

joaquimrocha left a comment

joaquimrocha Dec 2, 2024

farodin91 Dec 2, 2024

farodin91 Dec 2, 2024

farodin91 commented Dec 2, 2024

frontend: allow multiple deletes on ResourceTable #2594

Are you sure you want to change the base?

frontend: allow multiple deletes on ResourceTable #2594

Conversation

farodin91 commented Nov 21, 2024 • edited Loading

skoeva commented Nov 21, 2024

sniok left a comment

Choose a reason for hiding this comment

farodin91 commented Nov 22, 2024

joaquimrocha commented Nov 26, 2024

farodin91 commented Nov 26, 2024 • edited Loading

farodin91 commented Nov 26, 2024

farodin91 commented Nov 26, 2024

joaquimrocha commented Nov 26, 2024

farodin91 commented Nov 26, 2024

joaquimrocha commented Nov 27, 2024

farodin91 commented Nov 27, 2024

farodin91 commented Nov 27, 2024

joaquimrocha commented Nov 28, 2024

farodin91 commented Nov 30, 2024

joaquimrocha left a comment

Choose a reason for hiding this comment

joaquimrocha Dec 2, 2024

Choose a reason for hiding this comment

farodin91 Dec 2, 2024

Choose a reason for hiding this comment

farodin91 Dec 2, 2024

Choose a reason for hiding this comment

farodin91 commented Dec 2, 2024

farodin91 commented Nov 21, 2024 •

edited

Loading

farodin91 commented Nov 26, 2024 •

edited

Loading