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

Fixed not being able to drag nodes at same Y position as the toolbar #636

Merged
merged 2 commits into from
Jan 15, 2025

Conversation

AlexBxl
Copy link
Contributor

@AlexBxl AlexBxl commented Jan 14, 2025

User description

Description

  • Fixed not being able to drag nodes at same Y position as the toolbar

Fixes #617

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

Screenshots or video (if necessary):

a4764c1b-3704-4b8a-8546-2cb2237b412b


PR Type

Bug fix


Description

  • Fixed issue preventing node dragging behind toolbar.

  • Added pointerEvents: none to toolbar container style.

  • Ensured toolbar retains interactive pointer events.

  • Added changeset entry for the fix.


Changes walkthrough 📝

Relevant files
Bug fix
graph.tsx
Adjust toolbar container to allow interactions behind       

packages/graph-editor/src/editor/graph.tsx

  • Added pointerEvents: none to toolbar container style.
  • Ensured toolbar does not block interactions behind it.
  • +1/-0     
    toolbar.module.css
    Maintain toolbar interactivity with pointer-events adjustment

    packages/graph-editor/src/components/toolbar/toolbar.module.css

  • Set pointer-events: auto for toolbar itself.
  • Ensured toolbar remains interactive while container is transparent.
  • +1/-0     
    Documentation
    proud-balloons-remember.md
    Add changeset entry for toolbar drag fix                                 

    .changeset/proud-balloons-remember.md

  • Added changeset entry describing the bug fix.
  • Documented fix for dragging nodes behind toolbar.
  • +5/-0     

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

    Copy link

    changeset-bot bot commented Jan 14, 2025

    🦋 Changeset detected

    Latest commit: ae50f8b

    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

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    617 - Fully compliant

    Fully compliant requirements:

    • Fix the issue where nodes cannot be moved when they are behind the toolbar.
    • Ensure the toolbar remains interactive while resolving the issue.

    Not compliant requirements:
    None

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

    Pointer Events Adjustment

    Verify that adding pointerEvents: 'none' to the toolbar container style does not unintentionally disable necessary interactions with the toolbar.

    pointerEvents: 'none',
    Toolbar Interaction

    Confirm that setting pointer-events: auto in the toolbar CSS ensures proper interactivity without introducing unexpected behavior.

    pointer-events: auto;

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Verify that disabling pointer events does not interfere with other interactive elements in the same area

    Ensure that adding pointerEvents: 'none' does not unintentionally block interactions
    with other elements in the same area, as it could cause unexpected behavior in
    overlapping components.

    packages/graph-editor/src/editor/graph.tsx [824]

    -pointerEvents: 'none',
    +pointerEvents: 'none', // Ensure this does not block interactions with overlapping elements
    Suggestion importance[1-10]: 7

    Why: The suggestion is valid as it highlights a potential issue where disabling pointer events might inadvertently block interactions with overlapping elements. This is relevant to the change made in the PR, and ensuring this behavior is crucial for maintaining proper functionality.

    7
    Check that enabling pointer events for the toolbar does not block interactions with underlying elements

    Confirm that setting pointer-events: auto on .root does not reintroduce the issue of
    blocking interactions with elements beneath the toolbar.

    packages/graph-editor/src/components/toolbar/toolbar.module.css [11]

    -pointer-events: auto;
    +pointer-events: auto; /* Ensure this does not block interactions beneath the toolbar */
    Suggestion importance[1-10]: 7

    Why: The suggestion is pertinent because enabling pointer events on the toolbar could potentially reintroduce the issue of blocking interactions with elements beneath it. Verifying this behavior is important to ensure the fix does not cause regressions.

    7

    @@ -821,6 +821,7 @@ export const EditorApp = React.forwardRef<
    display: 'grid',
    placeItems: 'center',
    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    since we are touching this, can we move that css to the modules file?

    @mck mck merged commit 1751227 into master Jan 15, 2025
    3 checks passed
    @mck mck deleted the interact-behind-toolbar branch January 15, 2025 13:34
    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.

    impossible to move nodes from behind toolbar
    2 participants