frontend: add default actions to ResourceListView #2044
Conversation
|
@joaquimrocha What do you think of this idea? I could try to merge details and list actions. |
There was a problem hiding this comment.
Incredible changes @farodin91 ! Thanks a lot.
I left some comments which I believe will improve the code. I also noticed that the menu options partially outside the view. Not sure if you can e.g. render the menu with the anchor being the top right to avoid this.
Also. Once the review is approved, I think it would make sense to have the commits separated a bit. At least in 2: 1. Add actions capability to tables; 2. Add the view/edit/delete context actions to resource tables
| import { ConfirmDialog } from '../Dialog'; | ||
| import AuthVisible from './AuthVisible'; | ||
|
|
||
| interface DeleteButtonProps { | ||
| item?: KubeObject; | ||
| options?: CallbackActionOptions; | ||
| menuAction?: Boolean; | ||
| afterConfirm?: () => void; |
There was a problem hiding this comment.
I was confused with this one, as to why it's called afterConfirm and not onClick.
They I saw it's used effectively when the user confirms the dialog. But since this will be used mainly to close the menu, shouldn't we just use an onClick prop and close the menu right when the user clicks it?
It indeed looks a bit odd to me to see the menu open in the background of the dialog.
There was a problem hiding this comment.
I had the issue that delete didn't went through, if i close it with onclick.
There was a problem hiding this comment.
We have to work more dispatching that would be a major refactor in this PR of all actions.
| import { ConfirmDialog } from '../Dialog'; | ||
| import AuthVisible from './AuthVisible'; | ||
|
|
||
| interface DeleteButtonProps { | ||
| item?: KubeObject; | ||
| options?: CallbackActionOptions; | ||
| menuAction?: Boolean; |
There was a problem hiding this comment.
For booleans, usually it makes it easier for devs to guess that the property is a boolean when it is prefixed with is.
So isMenuItem would sound more intuitive.
A different way would be for the delete button to support a component which could take an enum OR a clickable component, so we could override whatever renders it. Using DeleteButtonRenderOptions.{Button|MenuItem} as built-in options.
But I do understand though that having this quick boolean property is convenient, so we can stick with isMenuItem if you prefer.
There was a problem hiding this comment.
I updated it a bit.
| { | ||
| id: DefaultHeaderAction.EDIT, | ||
| action: ({ item, closeMenu }) => ( | ||
| <EditButton item={item} menuAction afterConfirm={closeMenu} /> | ||
| ), | ||
| }, | ||
| { | ||
| id: DefaultHeaderAction.DELETE, | ||
| action: ({ item, closeMenu }) => ( | ||
| <DeleteButton item={item} menuAction afterConfirm={closeMenu} /> | ||
| ), | ||
| }, |
There was a problem hiding this comment.
For read-only roles, where users cannot delete nor edit resources, this will render the menu empty AFAIU.
One quick way to avoid this is to add a VIEW item. So at least we have an option in the menu. It's also something I have seen as an option sometimes, maybe it even improves a11y.
frontend/src/components/common/ActionMenuItem/ActionMenuItem.tsx
Outdated
Show resolved
Hide resolved
frontend/src/components/common/ActionMenuItem/ActionMenuItem.tsx
Outdated
Show resolved
Hide resolved
farodin91
force-pushed
the
frontend--idea-to-add-action-button-to-all-tables
branch
2 times, most recently
from
89b7e73
to
1808383
Compare
farodin91
changed the title
i used the inhouse method to render the menu. |
Signed-off-by: farodin91 <github@jan-jansen.net>
farodin91
force-pushed
the
frontend--idea-to-add-action-button-to-all-tables
branch
from
1808383
to
94c6cc6
Compare
|
@joaquimrocha I updated a lot of thinks. What do you think? |
No description provided.