feat: ability to edit user settings in bulk #597
Conversation
|
Btw not sure whether it's alright to change key names in the localization files due to refactoring a component out, so let me know if that's wrong. |
hmnd
force-pushed
the
feature/batch-update-users
branch
from
deab360
to
5f633b2
Compare
It should be fine but weblate tends to get have issues with merge conflicts when this happens. Maybe just modify only the |
|
@hmnd This PR is really fantastic! I have a few comments I will leave in the review but I also wanted to point out a design choice that I don't necessarily agree with. Screen.Recording.2021-01-08.at.21.51.03.movI really don't like how the whole table shifts here when clicking on this button. It can easily cause misclicks? And the effect is really jarring. What about moving the button to the right side of the table above the normal actions? You can even get away I think with using
What do you think? |
| @@ -47,6 +47,49 @@ router.get<{ id: string }>('/:id', async (req, res, next) => { | |||
| } | |||
| }); | |||
|
|
|||
| const canMakePermissionsChange = (permissions: number, user?: User) => | |||
There was a problem hiding this comment.
The name of this function is a little confusing. canMakePermissionsChange means that this should return true if the user can make a change, but it's actually the opposite. If we reverse the evaluations in the function to the opposite, the method name will make sense and then we can use it by checking if (!canMakePermissionChange) which would be a lot easier to understand.
src/components/UserList/index.tsx
Outdated
| @@ -175,6 +256,20 @@ const UserList: React.FC = () => { | |||
| <Table.TBody> | |||
| {data?.map((user) => ( | |||
| <tr key={`user-list-${user.id}`}> | |||
| <Table.TD> | |||
| {user.id !== currentUser?.id && ( | |||
There was a problem hiding this comment.
What do you think about not allowing modification of user id 1 here either? (And in the logic to select all users)
Totally agree. I couldn't think of a better place to put it that wouldn't move everything, but I think your idea is great! |
|
@sct Hmm how about one of these maybe? Neither causes any shift in the table. |
|
@hmnd Maybe "Bulk Edit" instead of only "edit"? |
|
I think it looks fine on the right? It's also where all the other edit buttons are. Yes, at lower resolutions that area gets cut off but the expected place for the edit buttons remains the same. Also, disabling the button instead of hiding it entirely makes sure the user knows its there even when they don't have anything checked: |
|
I do like @Magikarplvl4's idea of having it say Bulk Edit: |
|
@sct Yeah I think that works well! The button disappearing and reappearing was what put me off, but it works much better disabled like that. Will make those changes soon. :) |
|
Are you still working on this? |
|
@ankarhem yup, working on rebasing now :) |
Allow for multiple users to be edited at once in Users table. fix sct#492
hmnd
force-pushed
the
feature/batch-update-users
branch
from
5f633b2
to
f98f2eb
Compare
|
@sct should be ready for your review again! Sorry for the wait. |
|
🎉 This PR is included in version 1.18.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Description
Allow editing multiple users' settings at the same time in Users table.
Screenshot (if UI related)
Todos
yarn buildyarn i18n:extractIssues Fixed or Closed by this PR