Skip to content

add single node panel #707

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

add single node panel #707

wants to merge 1 commit into from

Conversation

mck
Copy link
Collaborator

@mck mck commented May 23, 2025

PR Type

Enhancement, Tests, Documentation


Description

  • Add unified NodePanel component layout

  • Implement inline and multiline editable fields

  • Integrate nodePanel into dropdown and toolbar

  • Add basic render test for NodePanel


Changes walkthrough 📝

Relevant files
Enhancement
3 files
ContentEditableText.tsx
Add ContentEditableText inline editing component                 
+106/-0 
ContentEditableTextarea.tsx
Add ContentEditableTextarea multiline editing component   
+124/-0 
index.tsx
Create unified NodePanel component layout                               
+202/-0 
Tests
1 files
test.tsx
Add basic NodePanel render test                                                   
+8/-0     
Configuration changes
4 files
layout.tsx
Add Node Panel item to layout dropdown                                     
+3/-0     
layoutButtons.tsx
Include NodePanel in toolbar layout buttons                           
+6/-0     
layoutController.tsx
Integrate nodePanel in layout controller                                 
+16/-10 
layout.ts
Integrate nodePanel in default UI layout                                 
+1/-10   
Documentation
1 files
README.md
Document unified Node Panel components                                     
+62/-0   

Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Ref Bug

    The ref handling in ContentEditableText is incorrect: elementRef is stored as an HTMLElement but later accessed as elementRef.current, which will always be undefined. This prevents blur logic (Enter/Escape) from working. Consider using useRef and passing the ref object directly.

    const [elementRef, setElementRef] = useState<HTMLElement | null>(null);
    
    useEffect(() => {
      setCurrentValue(value);
    }, [value]);
    
    // Set initial content and update when not editing to preserve cursor position
    useEffect(() => {
      if (elementRef) {
        if (!isEditing) {
          const displayValue = currentValue || placeholder;
          if (elementRef.textContent !== displayValue) {
            elementRef.textContent = displayValue;
          }
        }
      }
    }, [elementRef, currentValue, placeholder, isEditing]);
    
    // Set initial content on mount
    useEffect(() => {
      if (elementRef && !isEditing) {
        const displayValue = currentValue || placeholder;
        elementRef.textContent = displayValue;
      }
    }, [elementRef]);
    
    const handleFocus = useCallback(() => {
      setIsEditing(true);
    }, []);
    
    const handleBlur = useCallback(() => {
      setIsEditing(false);
      if (currentValue !== value) {
        onSave(currentValue);
      }
    }, [currentValue, value, onSave]);
    
    const handleInput = useCallback((e: React.FormEvent<HTMLElement>) => {
      const newValue = e.currentTarget.textContent || '';
      setCurrentValue(newValue);
    }, []);
    
    const handleKeyDown = useCallback(
      (e: React.KeyboardEvent) => {
        if (e.key === 'Enter' && !e.shiftKey) {
          e.preventDefault();
          elementRef.current?.blur();
        }
        if (e.key === 'Escape') {
          setCurrentValue(value);
          elementRef.current?.blur();
        }
      },
      [value],
    );
    Duplicate Logic

    There are two consecutive useEffect hooks (lines 31–40 and 43–48) performing the same initial content setup. Refactor or merge them to avoid redundancy and reduce maintenance overhead.

    useEffect(() => {
      if (elementRef) {
        if (!isEditing) {
          const displayValue = currentValue || placeholder;
          if (elementRef.textContent !== displayValue) {
            elementRef.textContent = displayValue;
          }
        }
      }
    }, [elementRef, currentValue, placeholder, isEditing]);
    
    // Set initial content on mount
    useEffect(() => {
      if (elementRef && !isEditing) {
        const displayValue = currentValue || placeholder;
        elementRef.textContent = displayValue;
      }
    }, [elementRef]);
    Accessibility

    The contentEditable element lacks ARIA roles and labels, which may hinder screen reader users. Consider adding role="textbox" and an appropriate aria-label or aria-labelledby.

    <Component
      ref={elementRef as React.RefObject<HTMLElement>}
      contentEditable
      suppressContentEditableWarning
      onFocus={handleFocus}
      onBlur={handleBlur}
      onInput={handleInput}
      onKeyDown={handleKeyDown}
      className={`${className} ${showPlaceholder ? 'placeholder' : ''}`}
      style={{
        outline: 'none',
        cursor: 'text',
        minHeight: '1.2em',
        ...style,
        ...(showPlaceholder && {
          color: 'var(--color-neutral-canvas-default-fg-muted)',
          fontStyle: 'italic',
        }),
      }}
      children={undefined}
    />

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    UseRef for element reference

    Use a ref hook instead of state to track the DOM element. This simplifies access to
    elementRef.current in handlers and ensures proper typing.

    packages/graph-editor/src/components/panels/node/ContentEditableText.tsx [24-84]

    -const [elementRef, setElementRef] = useState<HTMLElement | null>(null);
    +const elementRef = useRef<HTMLElement | null>(null);
     ...
    -ref={elementRef as React.RefObject<HTMLElement>}
    +ref={elementRef}
    Suggestion importance[1-10]: 9

    __

    Why: Using state for elementRef and casting it is incorrect and never sets the DOM node; switching to useRef ensures elementRef.current correctly references the element.

    High
    Remove clearing children prop

    Remove the children={undefined} prop so React does not clear the contentEditable DOM
    children on each render.

    packages/graph-editor/src/components/panels/node/ContentEditableText.tsx [102]

    -children={undefined}
    +// remove the children prop entirely
    Suggestion importance[1-10]: 6

    __

    Why: The children={undefined} prop is unnecessary and can cause React to clear the element’s content; removing it preserves the manually managed textContent.

    Low
    General
    Use event target to blur

    Use e.currentTarget.blur() to blur the element instead of relying on elementRef,
    avoiding inconsistent ref access.

    packages/graph-editor/src/components/panels/node/ContentEditableText.tsx [68-71]

     if (e.key === 'Enter' && !e.shiftKey) {
       e.preventDefault();
    -  elementRef.current?.blur();
    +  (e.currentTarget as HTMLElement).blur();
     }
    Suggestion importance[1-10]: 4

    __

    Why: Blurring via e.currentTarget is more reliable than using the ref and simplifies the handler, though it only addresses the Enter case.

    Low

    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.

    1 participant