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

Team edit modal #7043

Merged
merged 26 commits into from
May 9, 2023
Merged

Team edit modal #7043

merged 26 commits into from
May 9, 2023

Conversation

dieknolle3333
Copy link
Contributor

@dieknolle3333 dieknolle3333 commented May 2, 2023

Steps to test:

  • Go to teams list
  • Click on Add/Remove Users
  • Wait for modal to load (demo only)
  • click into modal
  • remove and add users
  • make sure changes are updating the next time the dropdown is opening

Issues:


(Please delete unneeded items, merge only when none are left open)

@dieknolle3333 dieknolle3333 self-assigned this May 5, 2023
@dieknolle3333 dieknolle3333 marked this pull request as ready for review May 5, 2023 12:24
@philippotto
Copy link
Member

I didn't look at the code yet, but already tested the PR out :) Some thoughts:

  • while removing/adding a user, there's no loading indicator. you can throttle your internet connection in the browser debugging tools to check this. I'd suggest to put everything in the modal inside of a <Spin spinning={pendingRequestsCounter > 0}>...</...> container and then increase/decrease pendingRequestsCounter before/after an add/remove request.
  • admin users cannot be removed from a team which makes sense, but I'd add a tooltip to the "Admin" label to explain this :)

Other than that it already feels quite good 👍 If the dropdown wouldn't close when clicking the actual name (see my other comment), this would be the cherry on the top 🍒

@dieknolle3333 dieknolle3333 marked this pull request as draft May 7, 2023 16:20
@dieknolle3333 dieknolle3333 marked this pull request as ready for review May 8, 2023 11:47
Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

excellent, it works really well 👍

only one last UI suggestion: please add a small "minus" icon button next to each user in the the users list (not in the dropdown) so that one can also use that button to remove a user from a team (without having to use the dropdown).

frontend/javascripts/admin/team/edit_team_modal_view.tsx Outdated Show resolved Hide resolved
Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

excellent, it works really well 👍

only one last UI suggestion: please add a small "minus" icon button next to each user in the the users list (not in the dropdown) so that one can also use that button to remove a user from a team (without having to use the dropdown).

export function renderUsersForTeam(
team: APITeam,
allUsers: APIUser[] | null,
renderAdditionalContent = Function(),
Copy link
Member

Choose a reason for hiding this comment

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

The Function() part is quite unusual in JS. Usually, it would be used to create a function from a string. Also new Function() would be cleaner. To avoid Function(), I would suggest this:

Suggested change
renderAdditionalContent = Function(),
renderAdditionalContent = (_teamMember: APIUser, _team: APITeam) => {},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright! This is a good reminder to double check what I'm reading on blogs 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why are you using the _syntax for _teamMember and _team?

Copy link
Member

Choose a reason for hiding this comment

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

Without the leading underscore signals the linter would complain about the unused function parameters. Adding an underscore to the name silents the linter :) The parameters are unused here, but we need them so that TS knows the type of the function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, thanks!

@philippotto
Copy link
Member

great! I got one last code suggestion and the todo/test comments/code can be removed now :)

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Beautiful :)

@dieknolle3333 dieknolle3333 merged commit 22914a1 into master May 9, 2023
2 checks passed
@dieknolle3333 dieknolle3333 deleted the team-edit-modal branch May 9, 2023 09:47
hotzenklotz added a commit that referenced this pull request May 17, 2023
…ty-list-drawings

* 'master' of github.com:scalableminds/webknossos: (23 commits)
  fix scrolling in organization switcher in terms-of-services modal (#7083)
  Auto-Select via SAM (#7051)
  Build STL in chunks when exporting them (#7074)
  Improve performance for large (oversegmentation) meshes (#7077)
  Fix display of used storage in power plan (#7057)
  Fix user limits for invites (#7078)
  adds fileSize to voxelytics workflow list (#7071)
  Improve error logging for bucket requests (#7053)
  Fix zarr streaming datasource-properties.json generation for non-wkw/zarr datasets (#7065)
  Min length for layer names is one (#7064)
  Include voxelytics workflow name in tab title (#7070)
  Fix local to global layer index look up (#7066)
  Merge editable mappings (#7026)
  store correct artifacts for wkorg nightly (#7060)
  Team edit modal (#7043)
  Fix voxel offset in chunk name for unsharded neuroglancer precomputed datasets (#7062)
  Fix offset when loading non-aligned buckets for zarr/n5/precomputed (#7058)
  fixes wallTimes query for workflow reports (#7059)
  Handle Remote Dataset Edge Cases: compressed content-encoding, float voxel size (#7041)
  Release 23.05.2 (#7056)
  ...
hotzenklotz added a commit that referenced this pull request May 17, 2023
…ove_wkconnect

* 'master' of github.com:scalableminds/webknossos: (33 commits)
  Fix bug in fallback rendering when layer has missing mags (#7082)
  Fix duplicate on volumes with no fallback layer (#7085)
  fix scrolling in organization switcher in terms-of-services modal (#7083)
  Auto-Select via SAM (#7051)
  Build STL in chunks when exporting them (#7074)
  Improve performance for large (oversegmentation) meshes (#7077)
  Fix display of used storage in power plan (#7057)
  Fix user limits for invites (#7078)
  adds fileSize to voxelytics workflow list (#7071)
  Improve error logging for bucket requests (#7053)
  Fix zarr streaming datasource-properties.json generation for non-wkw/zarr datasets (#7065)
  Min length for layer names is one (#7064)
  Include voxelytics workflow name in tab title (#7070)
  Fix local to global layer index look up (#7066)
  Merge editable mappings (#7026)
  store correct artifacts for wkorg nightly (#7060)
  Team edit modal (#7043)
  Fix voxel offset in chunk name for unsharded neuroglancer precomputed datasets (#7062)
  Fix offset when loading non-aligned buckets for zarr/n5/precomputed (#7058)
  fixes wallTimes query for workflow reports (#7059)
  ...
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.

Allow to add/remove users in teams view
2 participants