-
Notifications
You must be signed in to change notification settings - Fork 1
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
Feature/185 edit educators and volunteer #195
Conversation
…ithub.com/ISPP-G5/NexONG_Frontend into feature/185--edit-educators-and-volunteer
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.
Instead of a button as the green one you use, I think using an icon similar to has it is done with the delete would be better approach. Instead to be redirect to another screen when pressing the button a dialog tab could be open and it would be more efficient. A good approach would be also that the data from the user you want to change are shown by default and the warning image/icon use it not too appealing to the eyes (it is not really good seen)
I totally agree with Claudia. Also, you have written that the password is mandatory but I could change the info without having to write the password. And what password would it be, I guess admin's password because she is not going to have the password of the volunteer/educator/partner, maybe specify it. |
I have changed the bottom, now we are going to use the edit bottom, I have hidden the password and the mail because the admin does not need them. To show the info by default, it is necessary to change the Update Profile component, it could cause changes in other screens that used it. I would prefer not to changing it because I have not implemented it and I don't know it at all. I think that is better to assign that issue to the creator. |
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.
It allows to edit volunteers and educators and it is a good ide tor reuase the code for updating profiel (it saves implementation time). Just two small commtents:
1.I see no toast message of success when editing it is ok (which does not worry me that much, if you have reuse the code from the component the toast message has been fix on the feedback branch)
2. I read your comments regarding the info not been showed, try asking @fracalrod3 who did it but I think the info should be shown
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.
Seems fine overall and I think youy did right not totally changing updateProfile's functionality since it will cause trouble in other screens, I believe it's alright and it works as intended
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.
Everyting looks and good work fixing the errors and suggestions given to you. The idea you implement to reuse the component was a really goo one.
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.
Perfect, everything works!
…ithub.com/ISPP-G5/NexONG_Frontend into feature/185--edit-educators-and-volunteer
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.
Nice job with the conflicts
I have implemented the option to modify a user with role volunteer, educator, and partners. To do it, you need to click on the bottom editar, and you will be able to edit the profile.