-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add admin #176
Conversation
[diff-counting] Significant lines: 158. |
Visit the preview URL for this PR (updated for commit ea57c36): https://zing-lsc--pr176-add-admin-xpi182yo.web.app (expires Sun, 13 Aug 2023 00:59:18 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 1fc18307b178c916e2663810a6932f60b173c01b |
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.
Very nice fix! I tried playing around with it and didn't find anything bad. I really like how you had the undo button in the "confirmation state," but it's really big and stretches the table row (idk if that is intentional).
Is the edit feature going to be done in a future PR?
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 work Justin! I would just remove the edit icons and you should be good to go. I like how you accounted for duplicate emails.
# Conflicts: # frontend/src/modules/Settings/Components/AdministratorsTable.tsx # frontend/src/modules/Settings/Components/Settings.tsx # frontend/src/modules/Settings/Components/types.ts
fixed it up and merged main. |
Summary
This PR is the first step in making the full functionality to add admins to the allowed users list
Test Plan
The simple act of being able to add an admin is there
In this example, I was originally deleted
And then I filled myself out as a new administrator
After adding myself, I appear back on the administrators' table
Notes
Edit function still does not work but that can be overrided by removing an admin and then adding the same admin with updated information
Breaking Changes