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

Allow adding permisisons and identites when creatign a group. Simplify group editing #887

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

edlerd
Copy link
Collaborator

@edlerd edlerd commented Sep 10, 2024

Done

  • Allow adding permissions and identities when creating a group.
  • Simplify group editing

QA

  1. Run the LXD-UI:
  2. Perform the following QA steps:
    • create a new permission group
    • edit permission group: 1. add identities, 2. remove identities 3. add permissions 4. remove permissions

@webteam-app
Copy link

@mas-who
Copy link
Collaborator

mas-who commented Sep 11, 2024

Thanks for these changes! The interaction is a lot more intuitive now 👍 Below are some QA observations so far.

  1. More of a design question. The cancel and confirm buttons are hidden when adding identities and permissions currently, would it not be better to show them anyway? It seems a little weird that I always have to navigate back to the initial groups panel to save the changes.

  2. Another design question, when adding permissions during initial group creation, do we still want to strike out permissions when removing them? Since no permissions would have existed for the group at that point, is it maybe not a little weird?

  3. when trying to remove an identity from a group that is myself, after seeing the warning popup and dismissing it without confirming, the "Save changes" button is stuck in loading state.
    Screenshot from 2024-09-11 09-48-04
    Screenshot from 2024-09-11 09-48-33
    Screenshot from 2024-09-11 09-48-55

  4. The rows in the permission groups table seems to have some styling issues, could be caused by the margin on the edit button?
    Screenshot from 2024-09-11 09-43-14

  5. When removing an identity from a group that is not myself, seems like the warning popup still shows. Saving the changes results in the correct identity being removed though, so I think something might be wrong in the warning check. Actually, seems like if I do any modification to a group with myself in it, then saving changes will result in the warning popup to show.
    Screenshot from 2024-09-11 09-55-14
    Screenshot from 2024-09-11 09-56-14

  6. I see the undo/redo functionality is now removed in the identities and permissions panels when editing a group. I think you did mention this in a separate discussion, but I can't quite remember exactly why. Would you be able to remind me on the reasoning?

  7. Should we disable the "Save changes" button when editing a group if there are no modifications?

@edlerd
Copy link
Collaborator Author

edlerd commented Sep 11, 2024

Addressed 2, 3, 4 and 5. I think the others need a bit more discussion.

@edlerd edlerd force-pushed the group-edit branch 2 times, most recently from 1ff8232 to f948bcf Compare September 11, 2024 12:51
Copy link
Collaborator

@mas-who mas-who left a comment

Choose a reason for hiding this comment

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

Loved the approach with isAdded and isRemoved :) I have almost forgotten what permissions was all about. Left some comments for your reference

src/pages/permissions/panels/CreateGroupPanel.tsx Outdated Show resolved Hide resolved
src/pages/permissions/forms/PanelFormLink.tsx Outdated Show resolved Hide resolved
src/pages/permissions/forms/GroupForm.tsx Outdated Show resolved Hide resolved
src/pages/permissions/forms/PanelFormLink.tsx Outdated Show resolved Hide resolved
src/pages/permissions/panels/CreateGroupPanel.tsx Outdated Show resolved Hide resolved
src/pages/permissions/panels/EditGroupPanel.tsx Outdated Show resolved Hide resolved
@edlerd edlerd force-pushed the group-edit branch 3 times, most recently from 916d7a8 to 1565e6c Compare September 11, 2024 15:10
@edlerd
Copy link
Collaborator Author

edlerd commented Sep 11, 2024

Addressed all the comments, and the design points 1. and 7.

Only remaining is 6. which we probably best discuss in person. In short, the problem with undo is when switching between forms (a new interaction pattern for those panels), a generic undo stack for all 3 forms would be confusing and having 3 different undo stacks seems confusing as well.

@edlerd edlerd requested a review from mas-who September 11, 2024 15:22
@mas-who
Copy link
Collaborator

mas-who commented Sep 11, 2024

Addressed all the comments, and the design points 1. and 7.

Only remaining is 6. which we probably best discuss in person. In short, the problem with undo is when switching between forms (a new interaction pattern for those panels), a generic undo stack for all 3 forms would be confusing and having 3 different undo stacks seems confusing as well.

Thanks! I was thinking about 6. as I reviewed earlier, like you said I think it's not worth the complexity to include undo/redo as a feature here, let's leave it out 👍

@mas-who
Copy link
Collaborator

mas-who commented Sep 11, 2024

QA looks mostly good! Just one more thing, seems the "Cancel" and "Create group" buttons are still hidden when adding identities or permissions in the create group panel.

@mas-who
Copy link
Collaborator

mas-who commented Sep 11, 2024

thanks for the changes, QA and code both LGTM 👍

@edlerd
Copy link
Collaborator Author

edlerd commented Sep 11, 2024

@piperdeck can you have a look at this one? It tries to improve the experience on creation and editing of permission groups.

@edlerd edlerd force-pushed the group-edit branch 3 times, most recently from d8599cb to 8e66f95 Compare September 12, 2024 08:16
@edlerd edlerd force-pushed the group-edit branch 5 times, most recently from 234a4e1 to aa657ee Compare September 12, 2024 09:08
@edlerd edlerd requested a review from mas-who September 12, 2024 10:23
@mas-who
Copy link
Collaborator

mas-who commented Sep 12, 2024

The delete button for groups is cut off at the edge on smaller screen sizes.

Screenshot from 2024-09-12 14-49-40

@edlerd
Copy link
Collaborator Author

edlerd commented Sep 12, 2024

The delete button for groups is cut off at the edge on smaller screen sizes.

Good catch! Added a fix for it just now.

Copy link
Collaborator

@mas-who mas-who left a comment

Choose a reason for hiding this comment

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

Thanks for the improvement! Very minor code comments only. QA looks good!

src/pages/permissions/panels/EditGroupPanel.tsx Outdated Show resolved Hide resolved
src/util/permissions.tsx Show resolved Hide resolved
src/util/permissions.tsx Outdated Show resolved Hide resolved
…roup. Simplify group editing

Signed-off-by: David Edler <[email protected]>
@mas-who
Copy link
Collaborator

mas-who commented Sep 12, 2024

LGTM, again 🙂

@edlerd edlerd merged commit 0336e9e into canonical:main Sep 13, 2024
12 checks passed
@edlerd edlerd deleted the group-edit branch September 13, 2024 05:39
github-actions bot pushed a commit that referenced this pull request Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants