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

permissions selection with multiselect #94

Merged

Conversation

scicco
Copy link
Contributor

@scicco scicco commented Sep 20, 2024

Description

This commit will change permission management by using the vue-select library instead of checkboxes and address #95

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings

user_permissions

@zAlweNy26
Copy link
Member

Awesome! Thanks for the PR, I'll review it as soon as possible

@zAlweNy26
Copy link
Member

zAlweNy26 commented Sep 22, 2024

Hi @scicco , I tested the PR and saw some issues:

  • When I try to remove one permission, for some unknown reasons it removes the last two
  • It is not pretty visible in dark mode
  • The package is not made in typescript, making it difficult to use. I searched for an alternative online and saw this one which seems very good and more up-to-date although less well known.

Would you mind updating the PR to use this package instead?
Also please run the linting commands so that everything is prettified correctly.
Thanks!

@mastrogiovanni
Copy link

mastrogiovanni commented Sep 25, 2024

I would like to add a comment here: many providers use simple names to fill the matrix.
Instead of having a custom multiselect (that could be the "advanced mode") I suggest to use few words to summaryze multiple permissions:

None => ()
Reader => (READ, LIST)
Writer => (READ, LIST, WRITE, EDIT)
Admin => (READ, LIST, WRITE, EDIT, DELETE)

In this case a simple combo is enough

@zAlweNy26
Copy link
Member

@mastrogiovanni what if I want to switch to the "advanced mode"?

@mastrogiovanni
Copy link

@mastrogiovanni what if I want to switch to the "advanced mode"?

I image a simple flag on top, when checked is "advanced" mode.
In normal mode I would have only a single combo with that one I said.
Also I don't know if there are use cases for having a grid or a multiselect.

In reality the single combo solution is also a nice solution to switch permissions directly in the user list table.

@scicco
Copy link
Contributor Author

scicco commented Sep 28, 2024

Hi @scicco , I tested the PR and saw some issues:

* When I try to remove one permission, for some unknown reasons it removes the last two

* It is not pretty visible in dark mode

* The package is not made in typescript, making it difficult to use. I searched for an alternative online and saw [this one](https://vue3-select-component.vercel.app) che mi sembra ottimo e più aggiornato anche se meno conosciuto.

Would you mind updating the PR to use this package instead? Also please run the linting commands so that everything is prettified correctly. Thanks!

Hello, sorry for the delay in the response,

I'm having issues switching from the former component to the one you mentioned, mainly because I'm not well versed with vue and typescript in general: the new select expects having options with label and value while we simply use <Record<string: AuthPermission>> and I didn't find yet a proper way to manage that difference.

If I manage to resolve this, I'll also work on the other issues you mentioned

@scicco scicco force-pushed the feature/permissions-rework branch 2 times, most recently from 0726865 to 1d2df85 Compare September 28, 2024 20:28
@scicco scicco force-pushed the feature/permissions-rework branch 4 times, most recently from 5b4a478 to f5e3cc0 Compare October 9, 2024 17:46
@scicco scicco force-pushed the feature/permissions-rework branch from f5e3cc0 to 9f1a097 Compare October 9, 2024 17:48
@zAlweNy26 zAlweNy26 merged commit f771671 into cheshire-cat-ai:develop Oct 9, 2024
@pieroit
Copy link
Member

pieroit commented Oct 9, 2024

Kudos @scicco for the contribution and kudos @zAlweNy26 for the maintaining

Proud of you

@scicco scicco deleted the feature/permissions-rework branch October 9, 2024 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants