-
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
Updating Auth-Dependent UI Elements #34
Conversation
✅ Deploy Preview for wine-control ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for keen-syrniki-6d7c39 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
try { | ||
await logout(); | ||
} catch (error) { | ||
console.error('Logout error:', error); |
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.
Consider using the Mantine notification system, as implemented in issue #7. For examples of its usage, please refer to the corresponding branch.
For additional reference, visit:
https://mantine.dev/x/notifications/
const userLoggedIn = useIsAuthenticated(); | ||
|
||
const userInitials = | ||
userDetails?.name && userDetails?.lastname |
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 extract this into a function getUserInitials
)} | ||
</AppShell.Section> | ||
|
||
{userLoggedIn && ( |
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.
No need for this check, if user initials are falsy than default avatar picture will be shown
// TODO: Replace with the actual user data | ||
const userInitials = 'JD'; | ||
const { userUid } = useAuthService(); | ||
const { data: userDetails } = useGetUserDetailsQuery(userUid ?? skipToken); |
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.
Every query hooks has two data object data
and currentData
. Use currenData
with skipToken
, because if userUid
became falsy data
wil contain last query result from cache, but currenData
cache will be cleared.
For additional reference, visit:
reduxjs/redux-toolkit#3188
https://redux-toolkit.js.org/rtk-query/api/created-api/hooks#usequery
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.
Request for revision
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.
resolve #33
@@ -56,12 +65,18 @@ export const NavigationBar = () => { | |||
<NavLink | |||
label="Home" | |||
leftSection={<IconHome2 />} | |||
onClick={juxt([toggleSidebar, goDashboard])} | |||
onClick={ | |||
userLoggedIn ? juxt([toggleSidebar, goDashboard]) : undefined |
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.
Use disabled
prop
onClick={ | ||
userLoggedIn ? juxt([toggleSidebar, goDashboard]) : undefined | ||
} | ||
style={!userLoggedIn ? { color: 'gray' } : undefined} |
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.
No need for this if you use disabled
prop
/> | ||
<NavLink | ||
label="Settings" | ||
leftSection={<IconSettings />} | ||
onClick={juxt([toggleSidebar, goSettings])} | ||
onClick={ |
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.
Use disabled prop
onClick={ | ||
userLoggedIn ? juxt([toggleSidebar, goSettings]) : undefined | ||
} | ||
style={!userLoggedIn ? { color: 'gray' } : undefined} |
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.
No need for this if you use disabled prop
userUid ?? skipToken, | ||
); | ||
|
||
const { currentData: userLoggedIn } = useGetAuthStateQuery(); |
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.
Use data
, as currentData
is only and maybe useful when skipToken
or skip
option is used.
As the data returned from hook is an object
or undefined
and if you need a boolean
check its a good practice for clean stile to init new variable, for example:
const isLoggedIn = Boolean(data);
import { useSidebar } from 'features/layout/hooks/useSidebar'; | ||
|
||
export const Header = () => { | ||
const [isOpen, toggleSidebar] = useSidebar(); | ||
const [logout] = useLogoutMutation(); | ||
const { currentData: userLoggedIn } = useGetAuthStateQuery(); |
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.
Use data
, as currentData
is only and maybe useful when skipToken
or skip
option is used.
As the data returned from hook is an object
or undefined
and if you need a boolean
check its a good practice for clean stile to init new variable, for example:
const isLoggedIn = Boolean(data);
closes #33