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.

Sign up for GitHub

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

Jump to bottom

Feat/add user design #981

Merged
merged 12 commits into from
Nov 24, 2023
Merged

Feat/add user design #981

merged 12 commits into from
Nov 24, 2023

Conversation

tobi-bams
Copy link
Contributor

Describe your changes

Issue ticket number and link

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have tested on Chrome and Firefox
  • I have tested on a mobile device
  • I have provided a screenshot or recording of changes in my PR if there were updates to the frontend

@tobi-bams
Copy link
Contributor Author

Hi @ecurrencyhodler, here is the demo

@ecurrencyhodler
Copy link
Contributor

LGTM!

@tobi-bams tobi-bams linked an issue Nov 22, 2023 that may be closed by this pull request
Update "Add New Member" to Org Modal #764
Closed
@kevkevinpal
Copy link
Contributor

Is the search supposed to be working for me it isn't filtering out the users

@ecurrencyhodler
Copy link
Contributor

Yes search should be working. Thanks for catching this kev.

It should be similar to the "invite developer" in our bounty assigning flow minus the extra "skill" filter box:

Screen.Recording.2023-11-22.at.2.10.22.PM.mov

kevkevinpal
kevkevinpal reviewed Nov 22, 2023
View reviewed changes
Copy link
Contributor

@kevkevinpal kevkevinpal left a comment

Choose a reason for hiding this comment

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

added some comments I think we can add some tests for some logic we have in here but looking good, and I also cant filter the users by typing in the box not sure if that was out of scope

frontend/app/src/people/widgetViews/organization/AddUserModal.tsx Outdated Show resolved Hide resolved
frontend/app/src/people/widgetViews/organization/AddUserModal.tsx Outdated Show resolved Hide resolved
frontend/app/src/people/widgetViews/organization/AddUserModal.tsx Outdated Show resolved Hide resolved
frontend/app/src/people/widgetViews/organization/AssignUserRole.tsx Outdated Show resolved Hide resolved
frontend/app/src/people/widgetViews/organization/AssignUserRole.tsx Outdated Show resolved Hide resolved
frontend/app/src/people/widgetViews/organization/AssignUserRole.tsx Outdated Show resolved Hide resolved
frontend/app/src/people/widgetViews/organization/AssignUserRole.tsx Show resolved Hide resolved
frontend/app/src/people/widgetViews/organization/AssignUserRole.tsx Show resolved Hide resolved
frontend/app/src/people/widgetViews/organization/AssignUserRole.tsx Show resolved Hide resolved
@ecurrencyhodler
Copy link
Contributor

If it's difficult to test locally, let's merge and test on staging.

@elraphty
Copy link
Contributor

@ tobi-bams, is the flow to select all roles at first when the user roles modal pop up? But that's the case for some users, and use a default image on the user profile if the user image is not present.

AkhilKhan's Roles
Jeezman's Roles

@tobi-bams
Copy link
Contributor Author

@elraphty according to the issue here

Only, Manage bounties, fund organizaiton, withdraw from organization, and view txn history should be preselected.

I will add a default image thanks

@ecurrencyhodler
Copy link
Contributor

Yeah those 4 are the only pre-selected. Manage org and update members are sensitive so I want the person to intentionally select them.

kevkevinpal
kevkevinpal approved these changes Nov 24, 2023
View reviewed changes
Copy link
Contributor

@kevkevinpal kevkevinpal left a comment

Choose a reason for hiding this comment

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

looks good to me!

@kevkevinpal kevkevinpal merged commit e47bf76 into master Nov 24, 2023
5 checks passed
@ecurrencyhodler
Copy link
Contributor

ecurrencyhodler commented Nov 24, 2023 •
edited
Loading

@tobi-bams This looks great! I have one nitpick though. Adding the user should occur after we set user permissions. This video shows that they are added before they set user permissions.

edit: NVM. Let's forget this for now. We can change if users complain.

Screen.Recording.2023-11-24.at.3.54.23.PM.mov

elraphty pushed a commit that referenced this pull request Jan 26, 2024
@tobi-bams
Feat/add user design (#981)
a0fda2f 
* feat: add user to organization new flow

* feat: assign user roles

* update: made some db query change

* update: moved all styles in add user model to a new style file

* update: moved assign user role styles to style file

* update: updated syntax based on review

* feat: added search to find add user modal

* feat: added test for formatting roles that are displayed

* fix: fix import

* fix: moved function to another file

* update: added img for users without image
@Evanfeenstra Evanfeenstra deleted the feat/add-user-design branch January 26, 2024 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevkevinpal kevkevinpal kevkevinpal approved these changes

@elraphty elraphty Awaiting requested review from elraphty

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update "Add New Member" to Org Modal
4 participants
@tobi-bams @ecurrencyhodler @kevkevinpal @elraphty