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

"User groups" settings page #9081

Merged
merged 82 commits into from
May 9, 2024
Merged

"User groups" settings page #9081

merged 82 commits into from
May 9, 2024

Conversation

somebody1234
Copy link
Contributor

@somebody1234 somebody1234 commented Feb 16, 2024

Pull Request Description

Other changes:

  • Add delete button for users on "Members" settings page. Note that it currently does not work as corresponding backend functionality is missing.

Important Notes

None

Testing Instructions

Just the new "User Groups" page, and testing deletion of users in the "Members" page

Screenshots

User groups table:

  • The last group has two users, shown as indented rows.
  • Both users in the "Users" table below (to the right, if on a wider screen) are selected. These rows have the same styling as in the rest of the tables when not selected.
    image

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@somebody1234 somebody1234 added CI: No changelog needed Do not require a changelog entry for this PR. x-new-feature Type: new feature request g-dashboard labels Feb 16, 2024
@PabloBuchu
Copy link
Contributor

Sorry we made you waiting with this.

Member Roles

  1. No Roles but User Groups
  2. On group row make the same x as with the invitation emails (on hover). delete user group
DELETE /usergroups/usergrup-12313/
  1. 2nd table with users list (in format username (user email)
  2. Drag n drop user -> group to add user to that group.
PUT /users/{user_id}/usergroups {userGroups: [ids]}
  1. Users being appended to the list below the group. You can indent this sublist a bit

Members

  1. Add small x on the user same on hover

@somebody1234
Copy link
Contributor Author

member roles:

  1. done
  2. todo
  3. done, but i've decided to just use the same table as we are using on the members page instead. so two columns
  4. done, but there are minor style issues when a row for a usergroup is being hovered over, when dragging a user over the usergroup
  5. theoretically done, but i'm not able to test

members:

  1. todo, just like (2) on member roles. i think though, that we shouldn't be using the same x (mostly because table rows are a different size, partly because table rows can be very wide. i think it would make sense to have a "floating" (and larger) (x) to the left of the row. i imagine the appearance would be similar to the (+) buttons for the "shared with" and "labels" rows

@somebody1234
Copy link
Contributor Author

somebody1234 commented May 6, 2024

  1. (done) yeah i kinda forgot about this one because removing users from groups is still broken on the backend i think? which makes it a bit more difficult to debug... but not super hard so i'll take a look at it after addressing everything else
  2. (done) yeah makes sense. tbh the main reason it's gone is that it's placed past the right edge of the overflow container...
    • for now i'll just move it left so that it no longer gets clipped into non-existence
  3. (done) i guess we should disallow it. added to backend issue mentioned in part (4)
  4. (done) opened issue for backend as well:
  5. (done)
  6. (done) mb-16 is a lot! for now i've decided to use mb-4 lg:mb-8. that said... we're supposed to be using a shadow on the bottom to indicate if it is still scrollable
    • edit: replaced with shadow. it might be a bit too hard to see though...
  7. (idk?) yeah this is mainly an issue with CSS... i did try getting it to work for a long while but had no luck. maybe that was only an issue when i was using aria.Button as a workaround to get the tooltips to trigger.
    • upon further testing it seems like using ellipsis still breaks tooltips for some reason.

@MrFlashAccount
Copy link
Contributor

MrFlashAccount commented May 6, 2024

edit: replaced with shadow. it might be a bit too hard to see though...

Yea shadow is great, and together with margins should be an awesome solution

@MrFlashAccount
Copy link
Contributor

MrFlashAccount commented May 6, 2024

upon further testing it seems like using ellipsis still breaks tooltips for some reason.

I haven't seen your solution for tooltips yet, but I assume it's because they are inside the element with ellipsis applied. Moving it away from the container should fix the issue.

@somebody1234
Copy link
Contributor Author

together with margins should be an awesome solution

note that i've more or less removed margins for now.
pushed changes if you want to test (every point other than (7) should be addressed now).
i'm not against margins, but we do need to be careful that it doesn't make spacing look funky. also note that the scrollbar will extend into the margins as well, which (imo) looks more like a bug than a feature

@somebody1234
Copy link
Contributor Author

still no clue why point 7 is broken unfortunately

@MrFlashAccount
Copy link
Contributor

still no clue why point 7 is broken unfortunately

I did a fix in your branch, now it should be ok

@somebody1234
Copy link
Contributor Author

@MrFlashAccount doesn't work unfortunately. i've tried that before, and just tested it out on the current commit.
the tooltip is broken now, and the formatting too:
image

(no tooltip + blank line)

@somebody1234 somebody1234 force-pushed the wip/sb/groups-settings-page branch 3 times, most recently from 16ade8f to 06f25dd Compare May 7, 2024 08:36
@somebody1234 somebody1234 force-pushed the wip/sb/groups-settings-page branch from 06f25dd to 95629d5 Compare May 7, 2024 08:38
@MrFlashAccount
Copy link
Contributor

QA ✅

Nice work, @somebody1234 !

@somebody1234 somebody1234 added the CI: Ready to merge This PR is eligible for automatic merge label May 7, 2024
@mergify mergify bot merged commit 65179fb into develop May 9, 2024
34 of 36 checks passed
@mergify mergify bot deleted the wip/sb/groups-settings-page branch May 9, 2024 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge g-dashboard x-new-feature Type: new feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants