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: add default actions to ResourceListView #2044

Conversation

farodin91
Copy link
Contributor

@farodin91 farodin91 commented Jun 16, 2024

image

Now the actions can be used from the list views (by clicking the action ... to bring up a menu) as well as on the detail view.

@farodin91
Copy link
Contributor Author

@joaquimrocha What do you think of this idea? I could try to merge details and list actions.

Copy link
Collaborator

@joaquimrocha joaquimrocha left a comment

Choose a reason for hiding this comment

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

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.
Menu partially hidden

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

@illume illume marked this pull request as draft June 19, 2024 11:26
@farodin91 farodin91 force-pushed the frontend--idea-to-add-action-button-to-all-tables branch 2 times, most recently from 89b7e73 to 1808383 Compare July 1, 2024 20:01
@farodin91 farodin91 changed the title frontend: idea to add action button to all tables frontend: add default actions to ResourceListView Jul 1, 2024
@farodin91
Copy link
Contributor Author

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.

i used the inhouse method to render the menu.

@farodin91 farodin91 force-pushed the frontend--idea-to-add-action-button-to-all-tables branch from 1808383 to 94c6cc6 Compare July 1, 2024 20:10
@farodin91
Copy link
Contributor Author

@joaquimrocha I updated a lot of thinks. What do you think?

@illume illume requested a review from joaquimrocha October 8, 2024 11:00
@farodin91 farodin91 force-pushed the frontend--idea-to-add-action-button-to-all-tables branch 2 times, most recently from 0edb762 to b03c542 Compare October 24, 2024 20:36
@farodin91 farodin91 force-pushed the frontend--idea-to-add-action-button-to-all-tables branch from b03c542 to 40f8c20 Compare October 24, 2024 20:53
@farodin91 farodin91 marked this pull request as ready for review October 24, 2024 21:05
@dosubot dosubot bot added the size:XXL This PR changes 1000+ lines, ignoring generated files. label Oct 24, 2024
@farodin91
Copy link
Contributor Author

@joaquimrocha @illume Would you like to review?

Copy link
Collaborator

@illume illume left a comment

Choose a reason for hiding this comment

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

🎉 Thanks.

I tested it, and it's a really nice improvement.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Oct 25, 2024
@illume illume dismissed joaquimrocha’s stale review October 25, 2024 08:49

these changes were addressed

@illume illume added enhancement New feature or request frontend Issues related to the frontend labels Oct 25, 2024
@illume illume merged commit 96afea7 into headlamp-k8s:main Oct 25, 2024
15 checks passed
@joaquimrocha
Copy link
Collaborator

@farodin91 Another great PR! Thanks a lot, and keep those coming.

About this one, I think it's great as it is, but if we could hide the view button when the edit button is enabled (as its done through the AuthVisible comp in the details view), that would be more consistent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request frontend Issues related to the frontend lgtm This PR has been approved by a maintainer size:XXL This PR changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants