[WEB-1412]fix: split labels in kanban board #6253
Conversation
vamsikrishnamathala
added
🐛bug
WalkthroughThe pull request introduces several modifications to issue property management components, primarily focusing on label handling and dropdown interactions. A new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
web/core/components/issues/issue-layouts/properties/labels.tsx (1)
170-197: Handling empty label scenario cleanly.
When there are no labels, relying on the 'LabelDropdown' with a placeholder text improves consistency and user experience.Consider reusing the same approach (showing “x labels”) even when the count is 0 to reduce visual differences, especially if the user flow might benefit from uniform styling.
web/core/components/issues/issue-layouts/properties/label-dropdown.tsx (2)
103-114: Popper configuration with 'preventOverflow'.
This ensures the dropdown remains visible. Check for edge cases on extremely small viewports, though the default options appear sensible.
194-198: Propagation prevention inside the wrapper.
This pattern ensures the parent doesn't inadvertently capture the events.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
web/core/components/issues/issue-layouts/properties/all-properties.tsx(8 hunks)web/core/components/issues/issue-layouts/properties/index.ts(1 hunks)web/core/components/issues/issue-layouts/properties/label-dropdown.tsx(1 hunks)web/core/components/issues/issue-layouts/properties/labels.tsx(3 hunks)web/core/components/issues/issue-layouts/spreadsheet/columns/label-column.tsx(1 hunks)web/core/components/issues/workspace-draft/draft-issue-properties.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- web/core/components/issues/issue-layouts/properties/index.ts
- web/core/components/issues/workspace-draft/draft-issue-properties.tsx
🔇 Additional comments (21)
web/core/components/issues/issue-layouts/properties/labels.tsx (5)
6-16: Imports appear correct.
These newly added imports for "Tags", "Tooltip", "useLabel", "usePlatformOS", and "LabelDropdown" look consistent with the usage within this file.
36-36: New 'fullHeight' prop introduced in interface.
This prop broadens layout control. It's well-defined and optional. Ensure that other consuming components handle or ignore it as intended.
55-55: Default assignment to 'fullHeight'.
Assigning a default value of false aligns with a conservative approach that doesn't alter current layouts for existing users.
86-133: Refactored block to utilize ‘LabelDropdown’ for each label.
This significantly simplifies the code for rendering multiple labels. The separation of concerns into a dedicated dropdown is clearer. Good work!
134-168: Single 'LabelDropdown' usage when label count exceeds maxRender.
This fallback is a neat solution for handling numerous labels. The logic is straightforward and effectively reuses the same dropdown.
web/core/components/issues/issue-layouts/properties/label-dropdown.tsx (12)
16-33: Interface definition for 'LabelDropdown'.
Nice job defining a dedicated interface for clarity. The addition of “fullHeight” aligns well with the prop usage in “labels.tsx.”
59-64: Dropdown open state & query management.
Managing these as local states is a clean approach that isolates dropdown-specific concerns from parents.
73-80: Fetching labels and label creation.
Centralizing logic in 'useLabel' keeps the dropdown lean. Good approach.
84-99: Mapping project labels to ‘options’.
This pattern is straightforward. The “line-clamp” usage to handle potential overflow is a user-friendly touch.
115-119: 'onOpen' function fetches labels if not already available.
Great to see lazy loading of data only when needed.
120-125: ‘toggleDropdown’ logic.
Invoking 'onClose' when closing the dropdown fosters a cohesive two-way binding with the parent. Good use of callbacks.
133-141: 'handleAddLabel' allows dynamic label creation.
Please confirm that you have server-side validations for label conflicts or naming constraints to ensure consistent data integrity.
142-153: Search input key handling.
Preventing default events on “Enter” helps avoid undesired form submissions. Straightforward approach.
155-162: 'handleOnClick' preventing default event propagation.
This is best practice for handling nested clickable elements.
170-171: Dropdown button composition & popper referencing.
Using references from setReferenceElement and setPopperElement is a neat approach for flexible positioning.
Also applies to: 172-192
200-293: Conditional rendering of <Combobox.Options>
Code is well-structured, handling loading states, placeholders, and creation of new labels. The approach is consistent with Headless UI patterns.
35-54: Component signature and prop destructuring.
The destructuring approach is concise. Ensure that default values are thoroughly tested, especially when 'projectId' is null to avoid unexpected states.
Would you like to confirm usage patterns across the codebase with a quick search for “<LabelDropdown ” to ensure null projectId is handled gracefully?
✅ Verification successful
Let me check how projectId is used within the component to ensure proper null handling.
Null projectId handling is properly implemented.
The component has appropriate null checks for projectId in critical operations. The handleAddLabel function checks for projectId before proceeding (line 134), and the onOpen callback only fetches labels when both workspaceSlug and projectId are present (line 116). The component gracefully handles cases where projectId is null by using defaultOptions as a fallback (lines 81-82).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
rg -A 5 $'<LabelDropdown'
Length of output: 1806
Script:
#!/bin/bash
# Check the implementation of LabelDropdown to see how projectId is used
ast-grep --pattern 'const LabelDropdown = $_' -A 50 web/core/components/issues/issue-layouts/properties/label-dropdown.tsx
Length of output: 28624
web/core/components/issues/issue-layouts/spreadsheet/columns/label-column.tsx (1)
40-40: New ‘fullHeight’ prop usage in 'IssuePropertyLabels'.
This usage matches the interface changes in “labels.tsx,” expanding layout flexibility in the spreadsheet view.
web/core/components/issues/issue-layouts/properties/all-properties.tsx (3)
99-99: Explicit null check on 'updateIssue'.
Switching from a shorthand AND to an if-statement clarifies code intent.
114-114: Repeated checks before calling 'updateIssue'.
This pattern improves safety by ensuring the function exists before invoking it. The explicit approach is more readable and robust.
Also applies to: 129-129, 144-144, 198-198, 215-215, 232-232
309-317: Removing the extra
Simplifying the DOM structure is a nice improvement. No issues spotted.
vamsikrishnamathala
requested review from
SatishGandham,
rahulramesha and
sriramveeraghanta
| /> | ||
| ))} | ||
| </> | ||
| ) : ( |
There was a problem hiding this comment.
These conditions are a bit harder to read. Is it possible to refactor it a bit better?
chore: refactored label render component
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
web/core/components/issues/issue-layouts/properties/labels.tsx (1)
169-214: 🛠️ Refactor suggestionSimplify the nested conditional rendering for better readability.
Consider restructuring the conditional rendering to improve readability:
- {value.length > 0 ? ( - value.length <= maxRender ? ( - projectLabels - ?.filter((l) => value.includes(l?.id)) - .map((label) => ( - <LabelDropdown - key={label.id} - // ... props - /> - )) - ) : ( - <LabelDropdown - // ... props - label={LabelSummary} - /> - ) - ) : ( - <LabelDropdown - // ... props - label={NoLabel} - /> - )} + {(() => { + if (value.length === 0) { + return ( + <LabelDropdown + // ... props + label={NoLabel} + /> + ); + } + + if (value.length <= maxRender) { + return projectLabels + ?.filter((l) => value.includes(l?.id)) + .map((label) => ( + <LabelDropdown + key={label.id} + // ... props + /> + )); + } + + return ( + <LabelDropdown + // ... props + label={LabelSummary} + /> + ); + })()}
🧹 Nitpick comments (3)
web/core/components/issues/issue-layouts/properties/labels.tsx (1)
86-165: LGTM! Well-implemented label rendering with accessibility support.The memoized components and proper text truncation effectively address the label wrapping issue. Consider one optimization:
- [placeholderText, fullWidth, noLabelBorder, isMobile] + [placeholderText, fullWidth, noLabelBorder, isMobile, NoLabel]Add
NoLabelto the dependency array of the useMemo hook to prevent unnecessary re-renders when the component reference changes.web/core/components/issues/issue-layouts/properties/label-dropdown.tsx (2)
145-164: Add debouncing to search input for better performance.Consider debouncing the search input to reduce unnecessary re-renders:
+import { useMemo, useCallback, useRef } from 'react'; +import debounce from 'lodash/debounce'; const searchInputKeyDown = async (e: React.KeyboardEvent<HTMLInputElement>) => { e.stopPropagation(); if (query !== "" && e.key === "Escape") { setQuery(""); } if (query !== "" && e.key === "Enter") { e.preventDefault(); await handleAddLabel(query); } }; +const debouncedSetQuery = useMemo( + () => debounce((value: string) => setQuery(value), 300), + [] +); +useEffect(() => { + return () => { + debouncedSetQuery.cancel(); + }; +}, [debouncedSetQuery]);Then update the onChange handler:
-onChange={(e) => setQuery(e.target.value)} +onChange={(e) => debouncedSetQuery(e.target.value)}
211-304: Enhance accessibility with ARIA labels.Add ARIA labels to improve screen reader support:
<div className={`${fullHeight ? "h-full" : "h-5"}`} onClick={preventPropagation}> <ComboDropDown as="div" ref={dropdownRef} + aria-label="Label selector" className={`w-auto max-w-full h-full flex-shrink-0 text-left ${className}`} // ... other props > <Combobox.Input ref={inputRef} + aria-label="Search labels" className="w-full bg-transparent px-2 py-1 text-xs text-custom-text-200" // ... other props /> </ComboDropDown> </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
web/core/components/issues/issue-layouts/properties/all-properties.tsx(8 hunks)web/core/components/issues/issue-layouts/properties/label-dropdown.tsx(1 hunks)web/core/components/issues/issue-layouts/properties/labels.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/core/components/issues/issue-layouts/properties/all-properties.tsx
🔇 Additional comments (3)
web/core/components/issues/issue-layouts/properties/labels.tsx (2)
3-17: LGTM! Clean interface definition and well-organized imports.
The addition of the fullHeight prop and the organized imports improve code maintainability.
Also applies to: 19-36
Line range hint 38-70: LGTM! Efficient state management with proper hook usage.
The component effectively uses hooks and follows the observer pattern for reactive updates.
web/core/components/issues/issue-layouts/properties/label-dropdown.tsx (1)
1-88: LGTM! Well-structured component setup with comprehensive prop types.
The component initialization follows React best practices with proper type definitions and state management.
Description
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
Tested the functionality of isolated dropdowns to ensure they operate independently and correctly.
References
WEB-1412
Summary by CodeRabbit
New Features
LabelDropdowncomponent for managing issue labels.label-dropdownmodule.Improvements
IssuePropertiescomponent with explicit checks for updates.IssuePropertyLabelscomponent by removing unnecessary wrappers.fullHeightproperty to theIssuePropertyLabelscomponent for improved layout options.Bug Fixes
IssuePropertyLabelscomponent for better user experience.Chores