[WEB-2390] fix: Clickable Area for Issue List Layout Item#5536
[WEB-2390] fix: Clickable Area for Issue List Layout Item#5536
Conversation
WalkthroughThe changes involve refactoring multiple components by replacing existing structural elements with Changes
Possibly related PRs
Suggested labels
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 (
|
Jimmycutie
added
🐛bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- web/core/components/issues/issue-layouts/list/block.tsx (1 hunks)
Additional context used
Biome
web/core/components/issues/issue-layouts/list/block.tsx
[error] 199-199: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Additional comments not posted (1)
web/core/components/issues/issue-layouts/list/block.tsx (1)
136-294: Approve the use ofControlLinkto enhance interactivity.The introduction of the
ControlLinkcomponent wrapping theRowcomponent effectively makes the entire issue block clickable, aligning with the PR's objectives. The dynamic URL construction and event handling appear correctly implemented.Please ensure that the URL construction and event handling are thoroughly tested, especially the dynamic parts of the URL and the conditions under which the
onClickevent is triggered.Tools
Biome
[error] 199-199: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| <ControlLink | ||
| id={`issue-${issue.id}`} | ||
| href={`/${workspaceSlug}/projects/${issue.project_id}/${issue.archived_at ? "archives/" : ""}issues/${issue.id}`} | ||
| onClick={() => handleIssuePeekOverview(issue)} | ||
| className="w-full truncate cursor-pointer text-sm text-custom-text-100" | ||
| disabled={!!issue?.tempId} | ||
| > | ||
| <div className="flex w-full truncate"> | ||
| <div className="flex flex-grow items-center gap-0.5 truncate"> | ||
| <div className="flex items-center gap-1" style={isSubIssue ? { marginLeft } : {}}> | ||
| {/* select checkbox */} | ||
| {projectId && canSelectIssues && ( | ||
| <Tooltip | ||
| tooltipContent={ | ||
| <> | ||
| Only issues within the current | ||
| <br /> | ||
| project can be selected. | ||
| </> | ||
| } | ||
| disabled={issue.project_id === projectId} | ||
| renderByDefault={false} | ||
| > | ||
| <div className="flex-shrink-0 grid place-items-center w-3.5 absolute left-1"> | ||
| <MultipleSelectEntityAction | ||
| className={cn( | ||
| "opacity-0 pointer-events-none group-hover/list-block:opacity-100 group-hover/list-block:pointer-events-auto transition-opacity", | ||
| { | ||
| "opacity-100 pointer-events-auto": isIssueSelected, | ||
| } | ||
| )} | ||
| groupId={groupId} | ||
| id={issue.id} | ||
| selectionHelpers={selectionHelpers} | ||
| disabled={issue.project_id !== projectId} | ||
| /> | ||
| <Row | ||
| ref={issueRef} | ||
| className={cn( | ||
| "group/list-block min-h-11 relative flex flex-col gap-3 bg-custom-background-100 hover:bg-custom-background-90 py-3 text-sm transition-colors border border-transparent", | ||
| { | ||
| "border-custom-primary-70": getIsIssuePeeked(issue.id) && peekIssue?.nestingLevel === nestingLevel, | ||
| "border-custom-border-400": isIssueActive, | ||
| "last:border-b-transparent": !getIsIssuePeeked(issue.id) && !isIssueActive, | ||
| "bg-custom-primary-100/5 hover:bg-custom-primary-100/10": isIssueSelected, | ||
| "bg-custom-background-80": isCurrentBlockDragging, | ||
| "md:flex-row md:items-center": isSidebarCollapsed, | ||
| "lg:flex-row lg:items-center": !isSidebarCollapsed, | ||
| } | ||
| )} | ||
| onDragStart={() => { | ||
| if (!canDrag) { | ||
| setToast({ | ||
| type: TOAST_TYPE.WARNING, | ||
| title: "Cannot move issue", | ||
| message: "Drag and drop is disabled for the current grouping", | ||
| }); | ||
| } | ||
| }} | ||
| > | ||
| <div className="flex w-full truncate"> | ||
| <div className="flex flex-grow items-center gap-0.5 truncate"> | ||
| <div className="flex items-center gap-1" style={isSubIssue ? { marginLeft } : {}}> | ||
| {/* select checkbox */} | ||
| {projectId && canSelectIssues && ( | ||
| <Tooltip | ||
| tooltipContent={ | ||
| <> | ||
| Only issues within the current | ||
| <br /> | ||
| project can be selected. | ||
| </> | ||
| } | ||
| disabled={issue.project_id === projectId} | ||
| renderByDefault={false} | ||
| > | ||
| <div className="flex-shrink-0 grid place-items-center w-3.5 absolute left-1"> | ||
| <MultipleSelectEntityAction | ||
| className={cn( | ||
| "opacity-0 pointer-events-none group-hover/list-block:opacity-100 group-hover/list-block:pointer-events-auto transition-opacity", | ||
| { | ||
| "opacity-100 pointer-events-auto": isIssueSelected, | ||
| } | ||
| )} | ||
| groupId={groupId} | ||
| id={issue.id} | ||
| selectionHelpers={selectionHelpers} | ||
| disabled={issue.project_id !== projectId} | ||
| /> | ||
| </div> | ||
| </Tooltip> | ||
| )} | ||
| {displayProperties && displayProperties?.key && ( | ||
| <div className="flex-shrink-0" style={{ minWidth: `${keyMinWidth}px` }}> | ||
| {issue.project_id && ( | ||
| <IssueIdentifier | ||
| issueId={issueId} | ||
| projectId={issue.project_id} | ||
| textContainerClassName="text-xs font-medium text-custom-text-300" | ||
| /> | ||
| )} | ||
| </div> | ||
| </Tooltip> | ||
| )} | ||
| {displayProperties && displayProperties?.key && ( | ||
| <div className="flex-shrink-0" style={{ minWidth: `${keyMinWidth}px` }}> | ||
| {issue.project_id && ( | ||
| <IssueIdentifier | ||
| issueId={issueId} | ||
| projectId={issue.project_id} | ||
| textContainerClassName="text-xs font-medium text-custom-text-300" | ||
| /> | ||
| )} | ||
|
|
||
| {/* sub-issues chevron */} | ||
| <div className="size-4 grid place-items-center flex-shrink-0"> | ||
| {subIssuesCount > 0 && ( | ||
| <button | ||
| type="button" | ||
| className="size-4 grid place-items-center rounded-sm text-custom-text-400 hover:text-custom-text-300" | ||
| onClick={handleToggleExpand} | ||
| > | ||
| <ChevronRight | ||
| className={cn("size-4", { | ||
| "rotate-90": isExpanded, | ||
| })} | ||
| strokeWidth={2.5} | ||
| /> | ||
| </button> | ||
| )} | ||
| </div> | ||
| )} | ||
|
|
||
| {/* sub-issues chevron */} | ||
| <div className="size-4 grid place-items-center flex-shrink-0"> | ||
| {subIssuesCount > 0 && ( | ||
| <button | ||
| type="button" | ||
| className="size-4 grid place-items-center rounded-sm text-custom-text-400 hover:text-custom-text-300" | ||
| onClick={handleToggleExpand} | ||
| > | ||
| <ChevronRight | ||
| className={cn("size-4", { | ||
| "rotate-90": isExpanded, | ||
| })} | ||
| strokeWidth={2.5} | ||
| /> | ||
| </button> | ||
| {issue?.tempId !== undefined && ( | ||
| <div className="absolute left-0 top-0 z-[99999] h-full w-full animate-pulse bg-custom-background-100/20" /> | ||
| )} | ||
| </div> | ||
|
|
||
| {issue?.tempId !== undefined && ( | ||
| <div className="absolute left-0 top-0 z-[99999] h-full w-full animate-pulse bg-custom-background-100/20" /> | ||
| )} | ||
| </div> | ||
|
|
||
| {issue?.is_draft ? ( | ||
| <Tooltip | ||
| tooltipContent={issue.name} | ||
| isMobile={isMobile} | ||
| position="top-left" | ||
| disabled={isCurrentBlockDragging} | ||
| renderByDefault={false} | ||
| > | ||
| <p className="truncate">{issue.name}</p> | ||
| </Tooltip> | ||
| ) : ( | ||
| <ControlLink | ||
| id={`issue-${issue.id}`} | ||
| href={`/${workspaceSlug}/projects/${issue.project_id}/${issue.archived_at ? "archives/" : ""}issues/${ | ||
| issue.id | ||
| }`} | ||
| onClick={() => handleIssuePeekOverview(issue)} | ||
| className="w-full truncate cursor-pointer text-sm text-custom-text-100" | ||
| disabled={!!issue?.tempId} | ||
| > | ||
| {issue?.is_draft ? ( | ||
| <Tooltip | ||
| tooltipContent={issue.name} | ||
| isMobile={isMobile} | ||
| position="top-left" | ||
| disabled={isCurrentBlockDragging} | ||
| renderByDefault={false} | ||
| > | ||
| <p className="truncate">{issue.name}</p> | ||
| </Tooltip> | ||
| ) : ( | ||
| <Tooltip tooltipContent={issue.name} isMobile={isMobile} position="top-left" renderByDefault={false}> | ||
| <p className="truncate">{issue.name}</p> | ||
| </Tooltip> | ||
| </ControlLink> | ||
| )} | ||
| </div> | ||
| {!issue?.tempId && ( | ||
| <div | ||
| className={cn("block border border-custom-border-300 rounded", { | ||
| "md:hidden": isSidebarCollapsed, | ||
| "lg:hidden": !isSidebarCollapsed, | ||
| })} | ||
| > | ||
| {quickActions({ | ||
| issue, | ||
| parentRef: issueRef, | ||
| })} | ||
| )} | ||
| </div> | ||
| )} | ||
| </div> | ||
| <div className="flex flex-shrink-0 items-center gap-2"> | ||
| {!issue?.tempId ? ( | ||
| <> | ||
| <IssueProperties | ||
| className={`relative flex flex-wrap ${isSidebarCollapsed ? "md:flex-grow md:flex-shrink-0" : "lg:flex-grow lg:flex-shrink-0"} items-center gap-2 whitespace-nowrap`} | ||
| issue={issue} | ||
| isReadOnly={!canEditIssueProperties} | ||
| updateIssue={updateIssue} | ||
| displayProperties={displayProperties} | ||
| activeLayout="List" | ||
| /> | ||
| {!issue?.tempId && ( | ||
| <div | ||
| className={cn("hidden", { | ||
| "md:flex": isSidebarCollapsed, | ||
| "lg:flex": !isSidebarCollapsed, | ||
| className={cn("block border border-custom-border-300 rounded", { | ||
| "md:hidden": isSidebarCollapsed, | ||
| "lg:hidden": !isSidebarCollapsed, | ||
| })} | ||
| > | ||
| {quickActions({ | ||
| issue, | ||
| parentRef: issueRef, | ||
| })} | ||
| </div> | ||
| </> | ||
| ) : ( | ||
| <div className="h-4 w-4"> | ||
| <Spinner className="h-4 w-4" /> | ||
| </div> | ||
| )} | ||
| </div> | ||
| </Row> | ||
| )} | ||
| </div> | ||
| <div className="flex flex-shrink-0 items-center gap-2"> | ||
| {!issue?.tempId ? ( | ||
| <> | ||
| <IssueProperties | ||
| className={`relative flex flex-wrap ${isSidebarCollapsed ? "md:flex-grow md:flex-shrink-0" : "lg:flex-grow lg:flex-shrink-0"} items-center gap-2 whitespace-nowrap`} | ||
| issue={issue} | ||
| isReadOnly={!canEditIssueProperties} | ||
| updateIssue={updateIssue} | ||
| displayProperties={displayProperties} | ||
| activeLayout="List" | ||
| /> | ||
| <div | ||
| className={cn("hidden", { | ||
| "md:flex": isSidebarCollapsed, | ||
| "lg:flex": !isSidebarCollapsed, | ||
| })} | ||
| > | ||
| {quickActions({ | ||
| issue, | ||
| parentRef: issueRef, | ||
| })} | ||
| </div> | ||
| </> | ||
| ) : ( | ||
| <div className="h-4 w-4"> | ||
| <Spinner className="h-4 w-4" /> | ||
| </div> | ||
| )} | ||
| </div> | ||
| </Row> | ||
| </ControlLink> |
There was a problem hiding this comment.
Suggest simplifying conditional rendering logic for maintainability.
The extensive use of conditional rendering and state management through hooks is noted. While this is necessary for the component's functionality, consider simplifying the conditions to improve readability and maintainability.
Tools
Biome
[error] 199-199: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| </div> | ||
| </Tooltip> | ||
| )} | ||
| {displayProperties && displayProperties?.key && ( |
There was a problem hiding this comment.
Implement optional chaining to enhance code safety.
The static analysis tool suggests using optional chaining for accessing displayProperties. This change can prevent potential runtime errors and improve code robustness.
Consider applying this change:
- {displayProperties && displayProperties?.key && (
+ {displayProperties?.key && (Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {displayProperties && displayProperties?.key && ( | |
| {displayProperties?.key && ( |
Tools
Biome
[error] 199-199: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- web/core/components/issues/relations/issue-list-item.tsx (1 hunks)
- web/core/components/issues/sub-issues/issue-list-item.tsx (1 hunks)
Additional comments not posted (2)
web/core/components/issues/relations/issue-list-item.tsx (1)
93-172: Enhanced Clickable Area: Review of JSX Structure ChangeThe refactoring to wrap the entire issue block with
ControlLinkis correctly implemented. This change effectively makes the entire block clickable, which aligns with the PR's objective to enhance user interaction. TheControlLinkis appropriately placed to wrap all the relevant child components, ensuring that the entire area is responsive to clicks.The use of
ControlLinkwith anonClickhandler that triggershandleIssuePeekOverviewwhen the issue is clicked is a good practice, as it centralizes the click handling logic and makes it easier to manage.Overall, the changes are well-implemented and should improve the usability of the issue list item component.
web/core/components/issues/sub-issues/issue-list-item.tsx (1)
88-239: Simplified Interaction Model: Review of JSX Structure ChangeThe restructuring of the JSX elements to wrap the entire issue display area with
ControlLinkis effectively implemented. This change simplifies the component's hierarchy and makes the entire area clickable, which enhances the user interaction model as intended.The placement of
ControlLinkto encompass all child elements ensures that the click event is intuitively associated with the entire issue display, which is a significant improvement in terms of usability and user experience.The preservation of existing conditional rendering logic and the integration of
ControlLinkwithout altering the core functionality or visual elements is commendable. This ensures that the user experience remains consistent while improving the component's readability and maintainability.Overall, the changes are well-executed and align with the objectives of improving organization and readability of the code.
rahulramesha
left a comment
rahulramesha
left a comment
There was a problem hiding this comment.
@Jimmycutie The code changes look fine. Since you have moved the ControlLink to the parent level, could you please confirm if the following edge cases work fine,
- The dropdowns open and close just fine in all three components
- In case of the list layout the drag and drop works fine
- In case of list layout Opening of peek overview in draft issues
- In case of list layout selection of issues in EE work fine
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
web/core/components/workspace-notifications/sidebar/notification-card/item.tsx (1)
89-89: Approve the CSS class change to prevent overflow, but monitor readability.The change from
whitespace-normaltowhitespace-normal break-allwill allow words to break at arbitrary points to prevent overflow when the text is too long. This is a good solution to handle overflow issues.However, please monitor the impact on readability and user experience, as breaking words in the middle may sometimes affect legibility. If you find that the text becomes difficult to read in certain scenarios, consider exploring alternative solutions such as truncation or using a tooltip to display the full text on hover.
Yes, I have checked these edge cases and It's functioning fine. |
)" This reverts commit ed39f2d.
4 participants
Problem Statement
The current issue list layout only allows the title of each item to be clickable.
Solution
Moved Control function block to cover the whole issue block element instead of the inner title element.
Screenshots and Media
Web.Application.-.Issues.-.Google.Chrome.2024-09-05.20-01-40.mp4
References
[WEB-2390]
Summary by CodeRabbit
New Features
IssueBlockcomponent is now a clickable link that navigates to the issue overview, enhancing interactivity.RelationIssueListItemandIssueListItemcomponents have been updated to make the entire area clickable for improved user interaction.Bug Fixes
Improvements