fix: hidden settings menu affects settings layout #7769

Khaan25 · 2024-10-17T05:10:19Z

This PR fixes #6746

github-actions · 2024-10-17T05:11:07Z

Welcome!

Hello there, congrats on your first PR! We're excited to have you contributing to this project.
By submitting your Pull Request, you acknowledge that you agree with the terms of our Contributor License Agreement.

Generated by 🚫 dangerJS against c0c8f2d

greptile-apps

PR Summary

This pull request addresses an issue where hiding the left navigation menu affects the layout of the Settings page, causing it to shift to the left.

Added useIsRoute hook in packages/twenty-front/src/utils/currentPage.ts to check if the current route matches a given path
Modified PageHeader component in packages/twenty-front/src/modules/ui/layout/page/PageHeader.tsx to conditionally render the NavigationDrawerCollapseButton based on the current route
Prevents NavigationDrawerCollapseButton from rendering on the Settings page, fixing the layout issue when the left menu is hidden
Utilizes the new useIsRoute hook to determine if the current page is the Settings page

_{2 file(s) reviewed, 2 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2024-10-17T05:10:38Z

packages/twenty-front/src/modules/ui/layout/page/PageHeader.tsx


  return (
    <StyledTopBarContainer width={width}>
      <StyledLeftContainer>
-        {!isMobile && !isNavigationDrawerOpen && (
+        {!isMobile && !isNavigationDrawerOpen && !isSettingsPage && (


style: This condition now has multiple checks. Consider extracting it into a separate function for better readability

greptile-apps · 2024-10-17T05:10:52Z

packages/twenty-front/src/utils/currentPage.ts

+export const useIsRoute = (path: string): boolean => {
+  const location = useLocation();
+  return location.pathname === path;
+};


style: Consider using startsWith(path) instead of strict equality for more flexible route matching

charlesBochet · 2024-10-17T22:33:28Z

packages/twenty-front/src/utils/currentPage.ts

+ * @param path The route to match.
+ * @returns True if the current route matches the given path, false otherwise.
+ */
+export const useIsRoute = (path: string): boolean => {


a few feedbacks about this file:

we don't add JS doc in the project as we try to avoid comments and we are already using typescript to provide a nice dev xp. Function names should be clear enough to avoid doing it

useIsRoute is a hook so it should not be in a utils folder

I don't thin this s actually useful, we can just do useLocation().pathname === path whenever we need

So I would remove this hook actually

Right. I'll remove it.

I was also thinking the same

charlesBochet · 2024-10-17T22:34:13Z

packages/twenty-front/src/modules/ui/layout/page/PageHeader.tsx

@@ -109,11 +110,12 @@ export const PageHeader = ({
  const isMobile = useIsMobile();
  const theme = useTheme();
  const isNavigationDrawerOpen = useRecoilValue(isNavigationDrawerOpenState);
+  const isSettingsPage = useIsRoute('/settings/profile');


we should not only check for settings/profile but for all settings route

https://stackoverflow.com/questions/61235191/react-router-v6-check-if-path-matches-a-pattern

Thanks for the link, I'll have a look

charlesBochet

Thanks @Khaan25, I have left comments :)

charlesBochet · 2024-10-17T22:35:01Z

packages/twenty-front/src/modules/ui/layout/page/PageHeader.tsx

@@ -109,11 +110,12 @@ export const PageHeader = ({
  const isMobile = useIsMobile();
  const theme = useTheme();
  const isNavigationDrawerOpen = useRecoilValue(isNavigationDrawerOpenState);
+  const isSettingsPage = useIsRoute('/settings/profile');


https://stackoverflow.com/questions/61235191/react-router-v6-check-if-path-matches-a-pattern

Khaan25 · 2024-10-18T04:29:22Z

There you go! @charlesBochet

charlesBochet · 2024-10-18T07:38:05Z

packages/twenty-front/src/modules/ui/layout/page/PageHeader.tsx

-  const isNavigationDrawerExpanded = useRecoilValue(
-    isNavigationDrawerExpandedState,
-  );
+  const isNavigationDrawerOpen = useRecoilValue(isNavigationDrawerOpenState);


not sure why you changed the state name here

this is actually breaking the app

charlesBochet

@Khaan25 Thanks for pushing changes/

I have taken a new look and found that we actually already have a
useIsSettingsPage hook
fixing the state name (mentioned in comment), we are still having a blank Settings menu

Khaan25 · 2024-10-18T09:02:13Z

That's strange. I've tested it multiple times. Let me double check and get back to you.

Regarding the change, there was new code on the repo and I pulled in the latest changes.

Khaan25 · 2024-10-18T18:17:59Z

Here you go! @charlesBochet

It's fixed :) Let me know if there's any other issue with more points 😄You can assign me

2024-10-18.23-15-08.mp4

Khaan25 · 2024-10-19T16:11:09Z

Hey, could you review it? I can work on other issues?

Khaan25 · 2024-10-21T17:15:12Z

Hey @charlesBochet, could you please review :)

charlesBochet · 2024-10-21T18:08:16Z

On it :)

charlesBochet · 2024-10-21T18:11:17Z

...nt/src/modules/ui/navigation/navigation-drawer/components/NavigationDrawerCollapseButton.tsx

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


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

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

charlesBochet

Current behavior is functional but I'm not a fan of this useEffect

Khaan25 · 2024-10-21T18:15:56Z

I believe that we need to have this useEffect because it's gonna only trigger it when we've settings page :)

I really thought about solving without useEffect but I ran into bugs as we've had before.

charlesBochet · 2024-10-21T18:27:19Z

packages/twenty-front/src/modules/navigation/components/AppNavigationDrawer.tsx

@@ -48,10 +42,6 @@ export const AppNavigationDrawer = ({
        footer: <SupportDropdown />,
      };

-  useEffect(() => {


another useEffect spotted :p

Khaan25 · 2024-10-21T18:29:26Z

Really amazing job here. It opened my eyes that we could really remove it 🙌

charlesBochet · 2024-10-21T18:31:13Z

packages/twenty-front/src/modules/ui/utilities/hotkey/hooks/useGoToHotkeys.ts

@@ -5,13 +5,24 @@ import { AppHotkeyScope } from '../types/AppHotkeyScope';

 import { useSequenceHotkeys } from './useSequenceScopedHotkeys';

-export const useGoToHotkeys = (key: Keys, location: string) => {
+type GoToHotkeysProps = {


changing the API to take an object which is better from a dev XP as parameters are named when using the hook

Yes, I fully agree, Object is way more easier to debug and use.

I always to that :)

charlesBochet · 2024-10-21T18:31:43Z

packages/twenty-front/src/modules/app/effect-components/GotoHotkeysEffectsProvider.tsx

+import { useLocation } from 'react-router-dom';
+import { useRecoilCallback } from 'recoil';
+
+export const GotoHotkeysEffectsProvider = () => {


I'm actually only renaming this file from GoToHokeysEffect

charlesBochet · 2024-10-21T18:32:15Z

packages/twenty-front/src/modules/ui/utilities/hotkey/hooks/useGoToHotkeys.ts

+export const useGoToHotkeys = ({
+  key,
+  location,
+  preNavigateFunction,


passing a a preNavigationFunction to update the problematic state

Could you please explain me how this part is working?

I mean we did avoid useEffect but how's it solving the problem?

charlesBochet · 2024-10-21T18:32:52Z

@Khaan25 I have made some changes to avoid useEffects. Could you pull the changes and see if the functional behavior looks good?

Khaan25 · 2024-10-21T18:33:15Z

Sure, let me try

Khaan25 · 2024-10-21T19:01:04Z

Running the app locally is causing this issue:

charlesBochet · 2024-10-21T19:24:37Z

@Khaan25 I can't reproduce, what are you doing to get this error?

charlesBochet · 2024-10-21T19:27:41Z

I think we are good, I'll merge it :)

oss-gg · 2024-10-21T19:28:05Z

Awarding Khaan25: 150 points 🕹️ Well done! Check out your new contribution on oss.gg/Khaan25

Khaan25 · 2024-10-22T03:55:06Z

I just ran the twenty-front, maybe it's cache or something. Will try again and let you know it gives error again

fix: hidden settings menu affects settings layout

e394f7a

greptile-apps bot reviewed Oct 17, 2024

View reviewed changes

charlesBochet reviewed Oct 17, 2024

View reviewed changes

charlesBochet self-assigned this Oct 17, 2024

charlesBochet added the -PR: awaiting author label Oct 17, 2024

Khaan25 and others added 2 commits October 18, 2024 09:26

feat: improved the code

fa7c1ef

Merge branch 'main' into fix/hidden-menu-affects-settings-layout

e85cbd7

charlesBochet reviewed Oct 18, 2024

View reviewed changes

fix: side menu navigation

c0c8f2d

charlesBochet force-pushed the main branch from 1c484d5 to 2f72e22 Compare October 21, 2024 11:19

Merge branch 'main' into fix/hidden-menu-affects-settings-layout

6eaafce

charlesBochet reviewed Oct 21, 2024

View reviewed changes

Remove useEffects

3283c0d

charlesBochet reviewed Oct 21, 2024

View reviewed changes

Fix

1cf742e

charlesBochet reviewed Oct 21, 2024

View reviewed changes

charlesBochet approved these changes Oct 21, 2024

View reviewed changes

charlesBochet merged commit 5e2df81 into twentyhq:main Oct 21, 2024
16 checks passed

fix: hidden settings menu affects settings layout #7769

fix: hidden settings menu affects settings layout #7769

Conversation

Khaan25 commented Oct 17, 2024

github-actions bot commented Oct 17, 2024 • edited Loading

Welcome!

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Oct 17, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlesBochet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Khaan25 commented Oct 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlesBochet left a comment

Choose a reason for hiding this comment

Khaan25 commented Oct 18, 2024

Khaan25 commented Oct 18, 2024

Khaan25 commented Oct 19, 2024

Khaan25 commented Oct 21, 2024

charlesBochet commented Oct 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlesBochet left a comment

Choose a reason for hiding this comment

Khaan25 commented Oct 21, 2024

Choose a reason for hiding this comment

Khaan25 commented Oct 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlesBochet commented Oct 21, 2024 • edited Loading

Khaan25 commented Oct 21, 2024

Khaan25 commented Oct 21, 2024 • edited Loading

charlesBochet commented Oct 21, 2024

charlesBochet commented Oct 21, 2024

oss-gg bot commented Oct 21, 2024

Khaan25 commented Oct 22, 2024

github-actions bot commented Oct 17, 2024 •

edited

Loading

charlesBochet commented Oct 21, 2024 •

edited

Loading

Khaan25 commented Oct 21, 2024 •

edited

Loading