-
Notifications
You must be signed in to change notification settings - Fork 31
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 new user account page refactor #1285
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few things I see off the top.
src/main/webapp/resources/js/pages/admin/components/user/CreateNewUserForm.jsx
Outdated
Show resolved
Hide resolved
src/main/webapp/resources/js/pages/admin/components/user/CreateNewUserForm.jsx
Outdated
Show resolved
Hide resolved
src/main/webapp/resources/js/pages/admin/components/user/CreateNewUserForm.jsx
Outdated
Show resolved
Hide resolved
src/main/webapp/resources/js/pages/admin/components/user/CreateNewUserForm.jsx
Outdated
Show resolved
Hide resolved
src/main/webapp/resources/js/pages/admin/components/user/CreateNewUserForm.jsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is coming along really well. Just a couple more things
src/main/webapp/resources/js/pages/admin/components/user/CreateNewUserForm.jsx
Outdated
Show resolved
Hide resolved
src/main/webapp/resources/js/pages/admin/components/user/CreateNewUserForm.jsx
Outdated
Show resolved
Hide resolved
src/main/webapp/resources/js/pages/admin/components/user/CreateNewUserForm.jsx
Outdated
Show resolved
Hide resolved
src/main/webapp/resources/js/pages/admin/components/user/CreateNewUserForm.jsx
Outdated
Show resolved
Hide resolved
src/main/webapp/resources/js/pages/admin/components/AdminUsersPage.jsx
Outdated
Show resolved
Hide resolved
src/main/webapp/resources/js/pages/admin/components/AdminUsersPage.jsx
Outdated
Show resolved
Hide resolved
src/main/java/ca/corefacility/bioinformatics/irida/ria/web/users/UsersAjaxController.java
Outdated
Show resolved
Hide resolved
src/main/java/ca/corefacility/bioinformatics/irida/ria/web/users/UsersAjaxController.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still need to run the branch locally, but here are my code comments. I left one comment about assert argument ordering, this applies to all asserts added in this PR.
src/main/resources/ca/corefacility/bioinformatics/irida/database/all-changes.xml
Outdated
Show resolved
Hide resolved
src/main/webapp/resources/js/pages/admin/components/user/CreateNewUser.jsx
Show resolved
Hide resolved
...test/java/ca/corefacility/bioinformatics/irida/ria/unit/web/services/UIUsersServiceTest.java
Outdated
Show resolved
Hide resolved
src/main/webapp/resources/js/pages/admin/components/user/CreateNewUser.jsx
Outdated
Show resolved
Hide resolved
loading: submitting, | ||
onClick: form.submit, | ||
}} | ||
onCancel={() => setVisibility(false)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You close the modal you need to call form reset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see 3067d5f.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good to go now, thanks @ksierks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last few things. Otherwise it's good to go
Description of changes
Related issue
N/A
Checklist
Things for the developer to confirm they've done before the PR should be accepted: