Skip to content

Feature/reactui sidenav #402

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 12 commits into from
Apr 11, 2025
Merged

Feature/reactui sidenav #402

merged 12 commits into from
Apr 11, 2025

Conversation

AleksandarDev
Copy link
Member

@AleksandarDev AleksandarDev commented Apr 2, 2025

image

@AleksandarDev AleksandarDev requested a review from a team April 2, 2025 14:56
@AleksandarDev AleksandarDev enabled auto-merge April 10, 2025 11:12
Comment on lines +10 to +17
const params = useSearchParams();
const selectedItem = params.get('item');
const show = params.get('show') === 'true';
function setShow(show: boolean) {
const url = new URL(window.location.href);
url.searchParams.set('show', show.toString());
window.history.pushState({}, '', url.toString());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you choose native browser APIs over "Next.js's way" for any specific reason? I see that you're using Next's useSearchParams, so I think maybe you could integrate Next.js's routing all the way for consistency and reactivity

For example:

const router = useRouter();
const pathname = usePathname();
const params = useSearchParams();
const selectedItem = params.get('item');
const show = params.get('show') === 'true';

function setShow(show: boolean) {
    const params = new URLSearchParams(searchParams);
    params.set('show', show.toString());
    
    router.push(`${pathname}?${params.toString()}`);
    // router.push(`${pathname}?${params.toString()}`, { shallow: true }); If you don't want to run data fetching methods again
}

Comment on lines +45 to +46
const theme = useTheme();
const isMobile = useMediaQuery(theme.breakpoints.down('md'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could shorten this a bit by using callback function on a useMediaQuery hook that accepts the theme as the first argument, if you don't need that theme object anywhere else:

const isMobile = useMediaQuery((theme) => theme.breakpoints.down('md'));

export function SideNavItem({ children, href, selected, icon }: SideNavItemProps) {
const groupContext = useContext(SideNavItemGroupContext);
const child = groupContext?.inGroup;
const theme = useTheme();
Copy link
Member

@DorijanH DorijanH Apr 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need this theme object?

export function SideNavItemGroup({ children, label, expanded: controlledExpanded, defaultExpanded }: SideNavItemGroupProps) {
const [expanded, setExpanded] = useControllableState(controlledExpanded, defaultExpanded ?? false);
const handleToggleExpand = () => setExpanded(!expanded);
const theme = useTheme();
Copy link
Member

@DorijanH DorijanH Apr 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need this theme object?

@AleksandarDev AleksandarDev merged commit 2a51ed1 into stage Apr 11, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants