makeplane / plane

πŸ”₯ πŸ”₯ πŸ”₯ Open Source JIRA, Linear, Monday, and Asana Alternative. Plane helps you track your issues, epics, and product roadmaps in the simplest way possible.
http://plane.so
GNU Affero General Public License v3.0
30.88k stars 1.73k forks source link

[bug]: Wrong implementation of the projects list item #5986

Open oussamakhadraoui opened 2 weeks ago

oussamakhadraoui commented 2 weeks ago

Is there an existing issue for this?

Current behavior

Sidebar Projects List Component Optimization

Current Behavior

The current sidebar projects list implementation (SidebarProjectsListItem) has several areas that could be optimized for better performance, maintainability, and user experience.

Issues and Suggested Improvements

1. Performance Optimizations

// Suggested const dragDropDeps = useMemo( () => ({ projectRef: projectRef?.current, dragHandle: dragHandleRef?.current, projectId, isLastChild, projectListType }), [projectRef?.current, dragHandleRef?.current, projectId, isLastChild, projectListType] );


#### 2. Code Organization
- **Component Size**: The component is quite large (400+ lines) and handles multiple responsibilities. Consider splitting into smaller, focused components:
  ```typescript
  // Suggested structure
  - SidebarProjectsListItem/
    β”œβ”€β”€ index.tsx
    β”œβ”€β”€ ProjectHeader.tsx
    β”œβ”€β”€ ProjectNavigation.tsx
    β”œβ”€β”€ ProjectActions.tsx
    └── hooks/
        β”œβ”€β”€ useProjectDragDrop.ts
        └── useProjectNavigation.ts

3. Type Safety Improvements

4. Drag and Drop Logic

const dragHandleProps = {
  role: "button",
  "aria-label": "Drag to reorder project",
  "aria-grabbed": isDragging,
  "aria-dropeffect": "move"
};

5. Mobile Experience

// Suggested const { isMobile, breakpoint } = useResponsive(); if (isMobile) { toggleSidebar(); }


#### 6. State Management
- Consider using a reducer for complex state interactions:
```typescript
type ProjectState = {
  isMenuActive: boolean;
  isDragging: boolean;
  isOpen: boolean;
  instruction?: "DRAG_OVER" | "DRAG_BELOW";
};

type ProjectAction = 
  | { type: "TOGGLE_MENU" }
  | { type: "SET_DRAGGING"; payload: boolean }
  | { type: "SET_INSTRUCTION"; payload?: "DRAG_OVER" | "DRAG_BELOW" }
  | { type: "TOGGLE_OPEN" };

const projectReducer = (state: ProjectState, action: ProjectAction): ProjectState => {
  // reducer implementation
};

Technical Implementation

Here's a suggested approach for the refactor:

  1. Split Component:

    // ProjectHeader.tsx
    const ProjectHeader: FC<ProjectHeaderProps> = ({ project, isCollapsed }) => {
    const { logo, name } = project;
    return (
    <div className="flex items-center gap-2">
      <Logo logo={logo} size={16} />
      {!isCollapsed && <span className="truncate">{name}</span>}
    </div>
    );
    };
  2. Custom Hooks:

    // useProjectDragDrop.ts
    const useProjectDragDrop = (props: DragDropProps) => {
    const { projectId, handleDrop, disabled } = props;
    
    return {
    dragHandleProps,
    isDragging,
    dropIndicator
    };
    };

Testing Plan

  1. Unit tests for individual components
  2. Integration tests for drag-and-drop functionality
  3. Mobile responsiveness testing
  4. Performance benchmarking before/after refactor

Migration Strategy

  1. Create new component structure
  2. Add new components alongside existing ones
  3. Gradually migrate features
  4. Add comprehensive tests
  5. Remove old implementation

Questions

Tasks

/label enhancement, performance /label documentation /milestone v2.0.0

Steps to reproduce

Just follow the instruction

Environment

Production

Browser

Google Chrome

Variant

Cloud

Version

last version

SatishGandham commented 1 week ago

Hello @oussamakhadraoui,

Thank you for taking the time to report this issue. The suggested performance optimizations don’t seem applicable, as the component needs to re-render if any of these values change. Even if they were, the performance gains are unlikely to be significant, so this is not a current priority for us.

We'll keep the issue open for further review and will assess it in due course.

Let us know if you disagree.