Admin Bugbash Fixes - #206
Conversation
dburkhart07
left a comment
dburkhart07
left a comment
There was a problem hiding this comment.
just one small comment.
while we are at it, for adding a new user, can you also change "Please fill in all fields. " to not have the "" anymore?
| import { useAlert } from '../hooks/alert'; | ||
| import { getInitials, USER_ICON_COLORS } from '@utils/utils'; | ||
|
|
||
| const VolunteerManagement: React.FC = () => { |
There was a problem hiding this comment.
Can we just change this to UserManagement everywhere? Since its admin and volunteers, its kinda confusing like this.
Yurika-Kan
left a comment
Yurika-Kan
left a comment
There was a problem hiding this comment.
LGTM, one nit to address but then all good. thanks for fixing these, esp on adding the user feature :))
| const [isEditing, setIsEditing] = useState(false); | ||
|
|
||
| const donationId = donation.donationId; | ||
| const donationId = donation?.donationId; |
There was a problem hiding this comment.
nit: when we do optional chaining, it results in Donation | undefined, which becomes inconsistent with donation's form (Donation | null). can we clean it to
const donationId = donation ? donation.donationId : null;
Yurika-Kan
left a comment
Yurika-Kan
left a comment
There was a problem hiding this comment.
LGTM!!!! + fix idea on navbar error
a little on the FM Management bug in NavBar you mentioned: whole sidebar is gated on local currentUser state fetched independently inside Navbar.tsx, so any Amplify authStatus flicker (token refresh, Hub re-auth event) can cause the NavBar to flicker
what we can do inside of the navbar is to make the if else statement deterministically clear in Navbar.tsx:226-234
- only clear on definitive sign-out, not on transient states.
useEffect(() => {
if (authStatus === 'authenticated') {
ApiClient.getMe()
.then(setCurrentUser)
.catch(() => setCurrentUser(null));
} else if (authStatus === 'unauthenticated') {
setCurrentUser(null);
}
// 'configuring' -> keep last known currentUser, no flicker
}, [authStatus]);
3 participants
ℹ️ Issue
N/A
📝 Description
Note: the focus trap errors were intermittent, but I haven't been able to reproduce them since adding these fixes.
✔️ Verification
🏕️ (Optional) Future Work / Notes
Did you notice anything ugly during the course of this ticket? Any bugs, design challenges, or unexpected behavior? Write it down so we can clean it up in a future ticket!
Note: At one point, I found a bug where the "Manufacturer Management" page disappeared in the navbar. I tried for a while to reproduce this bug, but I wasn't able to, so I didn't implement a fix. Not sure if it's a recurring bug or if something strange just happened that time.