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.

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: RightDrawer doesn't save context values when clickedOutside #7729

Merged
merged 5 commits into from
Nov 6, 2024

Conversation

harshit078
Copy link
Contributor

@harshit078 harshit078 commented Oct 15, 2024

Description

Changes

Screen.Recording.2024-10-16.at.3.08.48.AM.mov

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR addresses an issue where the RightDrawer component wasn't saving context values when clicked outside, by implementing a debounced close function.

  • Introduced debounced closeRightDrawer function in packages/twenty-front/src/modules/ui/layout/right-drawer/components/RightDrawer.tsx to prevent immediate closure
  • Modified click outside listener callback to use the debounced close function
  • Imported debounce function from lodash to create the delayed closure mechanism
  • Adjusted RightDrawer component to maintain state of inputs when clicking outside
  • Resolved issue RightDrawer doesn't save context values when clickedOutside #7728 where cell container was clipping through the body on outside clicks

1 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile

@@ -9,7 +9,7 @@ import {
useSetRecoilState,
} from 'recoil';
import { Key } from 'ts-key-enum';

import debounce from 'lodash.debounce';
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider using a more specific import to reduce bundle size


emitRightDrawerCloseEvent();
}
},
[closeRightDrawer],
[debouncedCloseRightDrawer],
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Update dependency array to include closeRightDrawer

@bosiraphael bosiraphael self-assigned this Oct 16, 2024
@bosiraphael
Copy link
Contributor

Hello @harshit078, thank you for contributing :) Even if your solution works, I'm not sure that it is the right approach.
The bug happens because closeRightDrawer is a recoilCallback and is thus executed outside of the flow of react. I asked @lucasbordeau why it was that way and apparently it was needed for the email threads. I think the right solution is to rename closeRightDrawer to closeRightDrawerRecoilCallback inside useRightDrawer and to export closeRightDrawer like this:

const setIsRightDrawerOpen = useSetRecoilState(isRightDrawerOpenState);
const setIsRightDrawerMinimized = useSetRecoilState(
    isRightDrawerMinimizedState,
  );
 const closeRightDrawer = () => {
    setIsRightDrawerOpen(false);
    setIsRightDrawerMinimized(false);
  };

I'll let @charlesBochet and @lucasbordeau confirm though.

@charlesBochet
Copy link
Member

/award 200

Copy link

oss-gg bot commented Oct 31, 2024

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

@lucasbordeau
Copy link
Contributor

lucasbordeau commented Nov 6, 2024

@harshit078 We want to avoid debouncing as it is non deterministic.

Could you try with useListenRightDrawerClose ? Maybe in the drawer title component ?

@lucasbordeau
Copy link
Contributor

lucasbordeau commented Nov 6, 2024

@harshit078 When we need to debounce, it's generally a code smell because it means the underlying logic is too complex, we'll work on that to refactor this part, so the logic to persist a field can be deterministically and easily called before closing the right drawer.

@lucasbordeau
Copy link
Contributor

Follow-up issues : #8361 and #8360

@lucasbordeau lucasbordeau merged commit 18d04de into twentyhq:main Nov 6, 2024
17 checks passed
Copy link

github-actions bot commented Nov 6, 2024

Thanks @harshit078 for your contribution!
This marks your 36th PR on the repo. You're top 1% of all our contributors 🎉
See contributor page - Share on LinkedIn - Share on Twitter

Contributions

Copy link

sentry-io bot commented Nov 18, 2024

Sentry Issue: TWENTY-FRONT-2TZ

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RightDrawer doesn't save context values when clickedOutside
4 participants