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

Create user screen #395

Merged
merged 38 commits into from
Jun 3, 2022
Merged

Create user screen #395

merged 38 commits into from
Jun 3, 2022

Conversation

pkretzschmar
Copy link
Collaborator

@pkretzschmar pkretzschmar commented May 12, 2022

Resolves #320

@pkretzschmar pkretzschmar self-assigned this May 12, 2022
<div className="w-1/3 flex items-center gap-4">
<UserAvatar user={data} />
<span className=" font-mono text-sm">
{shortenEthAddress(data.ethereumAddress!)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

The user.ethereumAddress should always be defined (according to the mongoose entity).

Thus I think we can should modify the User & UserDto types to reflect this (make ethereumAddresss no longer optional). Then you won't need this non-null assertion.

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this collide with the needs of #361? If the User is created before account activation we don't know the eth address.

Copy link
Collaborator

@mattyg mattyg May 16, 2022

Choose a reason for hiding this comment

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

The user.ethereumAddress should always be defined (according to the mongoose entity).

That is outside the scope of this PR. When we're ready to address #361 there will need to be changes throughout the codebase -- we will address them all in a PR for #361.

Copy link
Collaborator

@mattyg mattyg left a comment

Choose a reason for hiding this comment

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

Thanks @pkretzschmar! This is great!!

I left a bunch of inline comments with requested changes (and a few things that weren't working).

Let me know if you want to go over any of this together! Or even pair-program some of it if you're interested! 😃

@kristoferlund
Copy link
Member

kristoferlund commented May 16, 2022

Yes, good stuff @pkretzschmar!

Just a little bit of polishing needed, then it's done.

mattyg
mattyg previously requested changes May 23, 2022
Copy link
Collaborator

@mattyg mattyg left a comment

Choose a reason for hiding this comment

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

Awesome! 🔥

I went through and made a bunch of minor changes:

  • deleted two components that are no longer needed
  • changed label of users page NavItem to "Users"
  • remove eslint ignore unused promises (see inline message response)
  • remove new dependency 'classnames' -- we've been using a local utility 'classNames' in utils/index.ts for the same functionality
  • hide ethereum address when it is identical to user name
  • remove frontend function getUsername -- instead use user.nameRealized (reflect refactoring done in Feat/user event log #365 while this PR was open)
  • remove frontend implementation of shortenEthAddress, use implementation (reflect refactoring done in Feat/user event log #365 while this PR was open)
  • prevent list of roles from breaking mid-word

I think there are just 3 issues remaining. I made screencasts to demonstrate them:

  • The pagination buttons do not change when the users list is limited to searched text:
Kazam_screencast_00003.mp4
  • The UserTable overlaps itself on small screens, and overflows the content area.
    Screenshot_2022-05-23_14-07-42

  • The role toggle buttons seem to be able to get into a weird state where they don't reflect the backend user.roles. I'm pretty sure this is related to use of useRecoilCallback within useAdminUsers. The frontend state setup is kinda confusing, so please let me know if I can help! We could also pair-program through this sometime if that's helpful!

Kazam_screencast_00001.mp4

@mattyg
Copy link
Collaborator

mattyg commented May 23, 2022

Also @pkretzschmar I know you've got other projects you're working on -- if you have limited availability to finish up this PR and #356 just let me know and I can pick up where you left off!

Copy link
Member

@kristoferlund kristoferlund left a comment

Choose a reason for hiding this comment

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

Yes! Nice work @pkretzschmar! I tweaked some paddings and made some other smaller fixes. Merging this now.

@kristoferlund kristoferlund dismissed mattyg’s stale review June 3, 2022 11:20

Requested changes fixed.

@kristoferlund kristoferlund merged commit 679e080 into main Jun 3, 2022
@kristoferlund kristoferlund deleted the feat/admin-screen branch June 3, 2022 11:20
@kristoferlund kristoferlund mentioned this pull request Jun 6, 2022
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.

Convert the Quantifier Pool screen to a User Admin screen
3 participants