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

Jump to bottom

fixed not being able to click out of command palette #641

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fixed not being able to click out of command palette #641

wants to merge 2 commits into from

Conversation

AlexBxl
Copy link
Contributor

@AlexBxl AlexBxl commented Jan 17, 2025 •
edited by github-actions bot
Loading

User description

Description

  • fixed not being able to click out of command palette

Fixes #541

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Video

51b9cf7e-79fe-4195-bb2c-218b6253fde4

PR Type

Bug fix

Description

  • Added a click outside handler for the Command Palette.

  • Used a ref to detect clicks outside the Command Palette.

  • Updated event listeners to manage the Command Palette visibility.

  • Added a changeset entry for the fix.

Changes walkthrough 📝

Relevant files
Bug fix
index.tsx
Add click outside handler for Command Palette                       

packages/graph-editor/src/components/commandPalette/index.tsx

  • Added a ref to the Command Palette dialog.
  • Implemented a useEffect hook to handle outside clicks.
  • Added event listeners to toggle Command Palette visibility.
    		• +22/-0   
    Documentation
    tricky-onions-care.md
    Add changeset entry for Command Palette fix                           

    .changeset/tricky-onions-care.md

  • Documented the fix for the Command Palette issue.
  • Added a patch entry for the graph-editor package.
    		• +5/-0     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @changeset-bot changeset-bot
    Copy link

    changeset-bot bot commented Jan 17, 2025 •
    edited
    Loading

    🦋 Changeset detected

    Latest commit: cae0742

    The changes in this PR will be included in the next version bump.

    This PR includes changesets to release 1 package
    Name Type
    @tokens-studio/graph-editor Patch

    Not sure what this means? Click here to learn what changesets are.

    Click here if you're a maintainer who wants to add another changeset to this PR

    @github-actions GitHub Actions
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    541 - Fully compliant

    Fully compliant requirements:

    • Fix the issue where the Command Palette cannot be closed by clicking outside of it.

    Not compliant requirements:
    []

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Event Listener Management

    Ensure that the event listener for mousedown is properly removed when the component unmounts or when showNodesCmdPalette changes to avoid potential memory leaks or unexpected behavior.

    React.useEffect(() => {
  const handleClickOutside = (event: MouseEvent) => {
    if (
      dialogRef.current &&
      !dialogRef.current.contains(event.target as HTMLElement)
    ) {
      dispatch.ui.setShowNodesCmdPalette(false);
    }
  };

  if (showNodesCmdPalette) {
    document.addEventListener('mousedown', handleClickOutside);
  }

  return () => {
    document.removeEventListener('mousedown', handleClickOutside);
  };

    @github-actions GitHub Actions
    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Use a more universal event listener to support touch and pointer devices

    Replace the mousedown event listener with pointerdown to ensure compatibility with
    touch devices and improve accessibility.

    packages/graph-editor/src/components/commandPalette/index.tsx [156]

    -document.addEventListener('mousedown', handleClickOutside);
+document.addEventListener('pointerdown', handleClickOutside);
    Suggestion importance[1-10]: 9

    Why: Replacing the mousedown event listener with pointerdown improves compatibility with touch devices and enhances accessibility. This change ensures the feature works seamlessly across a wider range of devices, which is highly beneficial for user experience.

    		9
    Possible issue
    Add a safety check to prevent runtime errors when calling a potentially undefined method

    Ensure that the handleClickOutside function checks if
    dispatch.ui.setShowNodesCmdPalette is defined before calling it, to prevent
    potential runtime errors if dispatch.ui or its method is undefined.

    packages/graph-editor/src/components/commandPalette/index.tsx [147-151]

     if (
   dialogRef.current &&
-  !dialogRef.current.contains(event.target as HTMLElement)
+  !dialogRef.current.contains(event.target as HTMLElement) &&
+  dispatch.ui?.setShowNodesCmdPalette
 ) {
   dispatch.ui.setShowNodesCmdPalette(false);
 }
    Suggestion importance[1-10]: 8

    Why: The suggestion adds a safety check to ensure that dispatch.ui.setShowNodesCmdPalette is defined before calling it, which is a good practice to prevent potential runtime errors. This is particularly important in dynamic environments where dispatch.ui might not always be initialized.

    		8

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers
    No reviews
    Assignees
    No one assigned
    Labels
    Review effort [1-5]: 2
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.

    Can't close command panel on outside click
    1 participant
    @AlexBxl