-
Notifications
You must be signed in to change notification settings - Fork 35
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
♻️ Refactor the tabs component to be reused #113
Conversation
Your Render PR Server URL is https://ozone-staging-pr-113.onrender.com. Follow its progress at https://dashboard.render.com/web/srv-coqfisn79t8c73cakg90. |
Your Render PR Server URL is https://ozone-sandbox-pr-113.onrender.com. Follow its progress at https://dashboard.render.com/web/srv-coqfit779t8c73cakgf0. |
components/shell/MobileMenu.tsx
Outdated
if (item.serviceAccountOnly && !isServiceAccount) { | ||
return | ||
} | ||
const Icon = ICONS[item.icon] |
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.
Does this end up showing the "Configure" nav item to non-service accounts? The main issue there is that only the service account has the ability to modify their service record, so it wont have the intended effect if another user does it.
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.
(There's also similar logic in SidebarNav.tsx)
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.
oh this wasn't meant to be committed. cleaned up.
components/common/Tabs.tsx
Outdated
const Tab = ({ | ||
view, | ||
label, | ||
sublabel, | ||
}: { |
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.
Defining a component anonymously like this has a quirk, which is that it always fully unmounts and remounts on render. I'd recommend just pulling this out into its own component outside of Tabs
render function.
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!
This is a simple refactor of the existing tabs components that was just copy pasted across various pages.