tweak(adminPanel): RN-1295: Update auth screens on admin panel #5666
Conversation
There was a problem hiding this comment.
More lines removed than added! 🤩
So much neater with TanStack Query—thanks for making that shift. I think my only substantial comment is about whether useUser should be a query hook or in a context. I’m also not sure if it’s change-worthy, so pre-approving regardless
The rest are just nitpicks
| return useMutation( | ||
| 'updateProfile', | ||
| payload => { | ||
| return put('me', { | ||
| data: payload, | ||
| }); | ||
| }, | ||
| { | ||
| onSuccess: () => { | ||
| queryClient.invalidateQueries(['user']); | ||
| }, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Question, sorry, what overload of useMutation is this? I trust it works, but can’t find it in the docs; I would’ve thought it’d need to look like
useMutation(
payload => put(...),
{
mutationKey: 'updateProfile',
onSuccess: () => {...},
}
)There was a problem hiding this comment.
Honestly I don't think it does anything, you're right. I will just remove, since we don't really need it sticking around anyway
There was a problem hiding this comment.
Ooh, this is clever
Though, is this something we would want in a context instead, like in DataTrak? Doesn’t look too tricky to figure out that, with this implementation, we’d just guard things behind isLoggedIn && ...; but I think if we can throw an error (and/or redirect to /login) when initialising a LoggedInUserContext it could guarantee a defined user
Not sure if that will disagree with other bits of the setup, though
| import { WHITE } from '../../theme/colors'; | ||
|
|
||
| export const UserLink = styled(Link)` | ||
| font-size: 0.6875rem; | ||
| export const UserLink = styled(Button).attrs({ |
There was a problem hiding this comment.
Hm, is it redundant to set both the color prop and CSS property?
I presume one trumps the other, but just reading it I’m not sure which
| {errorMessage && <ErrorMessage>{errorMessage}</ErrorMessage>} | ||
| {successMessage && <SuccessMessage>{successMessage}</SuccessMessage>} | ||
| {error && <ErrorMessage>{error.message}</ErrorMessage>} | ||
| {isSuccess && <SuccessMessage>Password successfully updated.</SuccessMessage>} |
There was a problem hiding this comment.
Does it make sense for this page to just regurgitate the success message from the backend?
There was a problem hiding this comment.
It definitely could, I was just copying over the existing logic
| <TextField | ||
| label="New Password" |
There was a problem hiding this comment.
| <TextField | |
| label="New Password" | |
| <TextField | |
| autoComplete="new-password" | |
| label="New Password" |
Autocomplete value for newPasswordConfirm would be identical
| <TextField | ||
| label="Current Password" |
There was a problem hiding this comment.
| <TextField | |
| label="Current Password" | |
| <TextField | |
| autoComplete="current-password" | |
| label="Current Password" |
| @@ -113,7 +93,7 @@ const ProfilePageComponent = React.memo(({ user, onUpdateProfile }) => { | |||
| <Divider /> | |||
| <TextField | |||
There was a problem hiding this comment.
given-name & family-name autocomplete values could also apply here ⚡
| @@ -180,7 +180,7 @@ const DataFetchingTableComponent = ({ | |||
| } | |||
| gotoPage(0); | |||
| setSortBy(defaultSorting ?? []); // reset sorting when table is re-initialised | |||
| }, [endpoint, baseFilter]); | |||
| }, [endpoint, JSON.stringify(baseFilter)]); | |||
There was a problem hiding this comment.
How does this affect the behaviour of useEffect? 😅
There was a problem hiding this comment.
baseFilter will be an object, and often if you pass in a nested object, checking the equality causes infinite re-renders and bugs. So stringifying it will stop that happening
| Don’t have an account?{' '} | ||
| {RegisterLinkComponent ? ( | ||
| RegisterLinkComponent | ||
| ) : ( | ||
| <RouterLink to={registerLink}>Sign up</RouterLink> | ||
| )} |
There was a problem hiding this comment.
This almost certainly fails Prettier formatting, but:
| Don’t have an account?{' '} | |
| {RegisterLinkComponent ? ( | |
| RegisterLinkComponent | |
| ) : ( | |
| <RouterLink to={registerLink}>Sign up</RouterLink> | |
| )} | |
| Don’t have an account?{' '} | |
| {RegisterLinkComponent ?? <RouterLink to={registerLink}>Sign up</RouterLink>} |
This reverts commit 302c829.
merge: update with latest dev before testing
merge: update with latest dev before testing
Issue RN-1295: Update auth screens on admin panel
Changes:
react-queryso it is easier to handle the auth flowScreenshots: