Skip to content
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.

Sign up for GitHub

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

fix: hidden settings menu affects settings layout #7769

Merged
merged 7 commits into from
Oct 21, 2024
Merged

fix: hidden settings menu affects settings layout #7769

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e394f7a
fix: hidden settings menu affects settings layout
Khaan25 Oct 17, 2024
fa7c1ef
feat: improved the code
Khaan25 Oct 18, 2024
e85cbd7
Merge branch 'main' into fix/hidden-menu-affects-settings-layout
Khaan25 Oct 18, 2024
c0c8f2d
fix: side menu navigation
Khaan25 Oct 18, 2024
6eaafce
Merge branch 'main' into fix/hidden-menu-affects-settings-layout
charlesBochet Oct 21, 2024
3283c0d
Remove useEffects
charlesBochet Oct 21, 2024
1cf742e
Fix
charlesBochet Oct 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/twenty-front/src/modules/ui/layout/page/PageHeader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { NavigationDrawerCollapseButton } from '@/ui/navigation/navigation-drawe

import { isNavigationDrawerExpandedState } from '@/ui/navigation/states/isNavigationDrawerExpanded';
import { useIsMobile } from '@/ui/utilities/responsive/hooks/useIsMobile';
import { matchPath, useLocation } from 'react-router-dom';

export const PAGE_BAR_MIN_HEIGHT = 40;

Expand Down
17 changes: 16 additions & 1 deletion ...src/modules/ui/navigation/navigation-drawer/components/NavigationDrawerCollapseButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import { useIsSettingsPage } from '@/navigation/hooks/useIsSettingsPage';
import { IconButton } from '@/ui/input/button/components/IconButton';
import { isNavigationDrawerExpandedState } from '@/ui/navigation/states/isNavigationDrawerExpanded';
import styled from '@emotion/styled';
import { useSetRecoilState } from 'recoil';
import { useEffect } from 'react';
import { useRecoilValue, useSetRecoilState } from 'recoil';
import {
IconLayoutSidebarLeftCollapse,
IconLayoutSidebarRightCollapse,
Expand All @@ -28,9 +30,22 @@ export const NavigationDrawerCollapseButton = ({
className,
direction = 'left',
}: NavigationDrawerCollapseButtonProps) => {
const isSettingsPage = useIsSettingsPage();

const setIsNavigationDrawerExpanded = useSetRecoilState(
isNavigationDrawerExpandedState,
);
const isNavigationDrawerExpanded = useRecoilValue(
isNavigationDrawerExpandedState,
);

// Check if the user is on the settings page and the navigation drawer is not expanded
// Then open it automatically
useEffect(() => {
if (!isNavigationDrawerExpanded && isSettingsPage) {
Copy link
Member

Choose a reason for hiding this comment

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

we don't put side effects in components as we don't want to encourage developers to implement behaviors during rendering (this is pushing them to take dependencies on variables and trigger re-renders). Instead, we either:

  • implement sync behaviors (in callbacks, event handlers, ...)
  • move useEffects to ComponentEffects that have no children so it's not a big deal from a performance perspective

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. I should be creating a custom hook to achieve it, right?

setIsNavigationDrawerExpanded(true);
}
}, [isSettingsPage]);

return (
<StyledCollapseButton
Expand Down
Loading