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: the "open new vault" button won't create a separate window #506

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

weilirs
Copy link
Collaborator

@weilirs weilirs commented Feb 26, 2025

Before:

Screen.Recording.2025-02-26.at.13.25.54.mov

After:

after.mov

Other fixes:

  • x button and clicking outside to close the modal

@weilirs weilirs requested a review from milaiwi February 26, 2025 18:33
Copy link

@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 fixes the "open new vault" button functionality and improves modal closing behavior across multiple components.

  • Fixed modal closing behavior in /src/components/Common/Modal.tsx by adding proper backdrop click detection and preventing event propagation
  • Added empty pointer event handlers to buttons in multiple components to prevent event propagation issues
  • Modified /src/components/Sidebars/IconsSidebar.tsx to show an in-app modal instead of opening a new window
  • Added onClose prop to InitialSetupSinglePage component for better modal management
  • Implemented proper state management for the vault setup modal with conditional rendering

💡 (3/5) Reply to the bot's comments like "Can you suggest a fix for this @greptileai?" or ask follow-up questions!

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

@@ -20,15 +20,17 @@ const ReorModal: React.FC<ModalProps> = ({

useEffect(() => {
const handleOffClick = (event: MouseEvent) => {
if (modalRef.current && !modalRef.current.contains(event.target as Node)) {
const isBackdropClick = event.target === document.querySelector(`.${tailwindStylesOnBackground}`)
Copy link

Choose a reason for hiding this comment

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

logic: Check if tailwindStylesOnBackground is undefined before using it in the querySelector.

Suggested change
const isBackdropClick = event.target === document.querySelector(`.${tailwindStylesOnBackground}`)
const isBackdropClick = tailwindStylesOnBackground ? event.target === document.querySelector(`.${tailwindStylesOnBackground}`) : event.target === event.currentTarget

Comment on lines +75 to +76
onPointerEnterCapture={() => {}}
onPointerLeaveCapture={() => {}}
Copy link

Choose a reason for hiding this comment

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

style: Empty event handlers added here but not used. Consider removing if not needed or document why they're required for the Material Tailwind Button component.

Suggested change
onPointerEnterCapture={() => {}}
onPointerLeaveCapture={() => {}}

@milaiwi
Copy link
Collaborator

milaiwi commented Feb 28, 2025

Mostly looks good! One thing though is maybe downgrade @types/react to @18.2.19 and then remove all the onPointerEnterCapture and onPointerLeaveCapture.

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

Successfully merging this pull request may close these issues.

2 participants