-
Couldn't load subscription status.
- Fork 1
Enhanced sidebar and Implement horizontal tab navigation for mobile settings screen || Admin Portal #341
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
base: main
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis PR implements responsive navigation for the settings screen by introducing a horizontal menu for mobile devices while maintaining vertical tabs for larger screens. The implementation uses Ant Design's Grid breakpoints to detect mobile devices and conditionally renders either a horizontal Menu component or the existing VerticalTabs component. No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @dev-kishor - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider expanding the mobile view breakpoint check beyond just 'xs' - you might want to use 'sm' or a custom breakpoint to better accommodate various device sizes and orientations.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Review instructions: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| </Helmet> | ||
| <VerticalTabs pages={pages} /> | ||
| </Helmet> | ||
| {isMobile ? ( |
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.
issue (complexity): Consider extracting routing logic into a shared component to avoid duplication between mobile and desktop views
The current implementation duplicates routing logic between VerticalTabs and explicit Routes. Consider extracting the Routes to a shared component and only varying the navigation UI:
export const Settings: React.FC<any> = () => {
// ... existing setup code ...
const routeElements = (
<Routes>
<Route path="/" element={<Navigate to="general" replace />} />
{pages.map((page) => (
<Route key={page.id} path={page.path} element={page.element} />
))}
</Routes>
);
return (
<SubPageLayout fixedHeader={false} header={...}>
<Helmet>
<title>Admin Settings</title>
</Helmet>
<Col>
{isMobile ? (
<Row>
<Menu className="w-11/12" items={profileNavigationMenu} mode="horizontal" style={{ borderColor: '#3f4373' }} />
</Row>
) : (
<VerticalTabs pages={pages} />
)}
<Row style={{ width: '100%', padding: '24px 0px 0px 24px' }}>
{routeElements}
</Row>
</Col>
</SubPageLayout>
);
};This approach:
- Consolidates routing logic in one place
- Maintains separate mobile/desktop navigation UIs
- Reduces code duplication and maintenance overhead
…or improved performance
Introduce a horizontal menu for mobile devices on the settings screen, enhancing navigation experience while maintaining vertical tabs for larger screens.
Summary by Sourcery
Implement horizontal tab navigation for the settings screen on mobile devices, enhancing the user experience by providing a more suitable navigation method for smaller screens while retaining the existing vertical tabs for larger screens.
New Features:
Enhancements: