[WEB-1936] fix: flicker issue in issues list layout#5412
[WEB-1936] fix: flicker issue in issues list layout#5412SatishGandham merged 4 commits intopreviewfrom
Conversation
WalkthroughThe recent changes introduce optional properties to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant IssueBlocksList
participant IssueBlockRoot
User->>IssueBlocksList: Request to display issues
IssueBlocksList->>IssueBlockRoot: Check creation timestamp
alt Created within last 30 seconds
IssueBlocksList->>IssueBlockRoot: Render with shouldRenderByDefault = true
else Created more than 30 seconds ago
IssueBlocksList->>IssueBlockRoot: Render with shouldRenderByDefault = false
end
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 as PR comments)
Additionally, you can add 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
Outside diff range, codebase verification and nitpick comments (1)
web/core/components/issues/issue-layouts/list/blocks-list.tsx (1)
45-46: Consider renamingshouldRenderByDefaultfor clarity.The variable name
shouldRenderByDefaultcould be more descriptive to indicate its purpose, such asisRecentlyCreated.- const shouldRenderByDefault = + const isRecentlyCreated =
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- web/core/components/core/render-if-visible-HOC.tsx (2 hunks)
- web/core/components/issues/issue-layouts/list/block-root.tsx (4 hunks)
- web/core/components/issues/issue-layouts/list/blocks-list.tsx (1 hunks)
Additional comments not posted (3)
web/core/components/issues/issue-layouts/list/blocks-list.tsx (1)
48-70: Conditional rendering logic is well-implemented.The conditional rendering of
IssueBlockRootbased on the issue's creation time is a good enhancement for highlighting recently created issues.web/core/components/core/render-if-visible-HOC.tsx (1)
Line range hint
13-28: The addition ofdefaultValueis a valuable enhancement.Using
defaultValueto initialize theshouldVisiblestate enhances the component's flexibility and usability.web/core/components/issues/issue-layouts/list/block-root.tsx (1)
Line range hint
39-171: The addition ofshouldRenderByDefaultenhances configurability.The
shouldRenderByDefaultprop allows for more nuanced rendering based on the component's state, improving the component's flexibility.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- web/core/components/issues/issue-layouts/list/blocks-list.tsx (2 hunks)
- web/core/components/issues/issue-layouts/utils.tsx (1 hunks)
Additional comments not posted (1)
web/core/components/issues/issue-layouts/utils.tsx (1)
616-621: Verify date handling inisIssueNewfunction.The logic for determining if an issue is new is correct, but consider handling edge cases such as invalid dates or time zone differences. Ensure that
issue.created_atis always a valid date string.Run the following script to check for invalid
created_atvalues:
| issueIds.map( | ||
| (issueId: string, index: number) => | ||
| issuesMap[issueId].created_at && ( | ||
| <IssueBlockRoot | ||
| key={issueId} | ||
| issueIds={issueIds} | ||
| issueId={issueId} | ||
| issuesMap={issuesMap} | ||
| updateIssue={updateIssue} | ||
| quickActions={quickActions} | ||
| canEditProperties={canEditProperties} | ||
| displayProperties={displayProperties} | ||
| nestingLevel={0} | ||
| spacingLeft={0} | ||
| containerRef={containerRef} | ||
| selectionHelpers={selectionHelpers} | ||
| groupId={groupId} | ||
| isLastChild={index === issueIds.length - 1} | ||
| isDragAllowed={isDragAllowed} | ||
| canDropOverIssue={canDropOverIssue} | ||
| shouldRenderByDefault={isIssueNew(issuesMap[issueId])} | ||
| /> | ||
| ) | ||
| )} |
There was a problem hiding this comment.
Consider extracting the rendering logic into a separate function.
The rendering logic for IssueBlockRoot could be moved to a separate function to enhance reusability and readability, as previously suggested by another reviewer.
+const renderIssueBlock = (issueId, index) => (
+ issuesMap[issueId].created_at && (
+ <IssueBlockRoot
+ key={issueId}
+ issueIds={issueIds}
+ issueId={issueId}
+ issuesMap={issuesMap}
+ updateIssue={updateIssue}
+ quickActions={quickActions}
+ canEditProperties={canEditProperties}
+ displayProperties={displayProperties}
+ nestingLevel={0}
+ spacingLeft={0}
+ containerRef={containerRef}
+ selectionHelpers={selectionHelpers}
+ groupId={groupId}
+ isLastChild={index === issueIds.length - 1}
+ isDragAllowed={isDragAllowed}
+ canDropOverIssue={canDropOverIssue}
+ shouldRenderByDefault={isIssueNew(issuesMap[issueId])}
+ />
+ )
+);
return (
<div className="relative h-full w-full">
{issueIds && issueIds.length > 0 && issueIds.map(renderIssueBlock)}
</div>
);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.
| issueIds.map( | |
| (issueId: string, index: number) => | |
| issuesMap[issueId].created_at && ( | |
| <IssueBlockRoot | |
| key={issueId} | |
| issueIds={issueIds} | |
| issueId={issueId} | |
| issuesMap={issuesMap} | |
| updateIssue={updateIssue} | |
| quickActions={quickActions} | |
| canEditProperties={canEditProperties} | |
| displayProperties={displayProperties} | |
| nestingLevel={0} | |
| spacingLeft={0} | |
| containerRef={containerRef} | |
| selectionHelpers={selectionHelpers} | |
| groupId={groupId} | |
| isLastChild={index === issueIds.length - 1} | |
| isDragAllowed={isDragAllowed} | |
| canDropOverIssue={canDropOverIssue} | |
| shouldRenderByDefault={isIssueNew(issuesMap[issueId])} | |
| /> | |
| ) | |
| )} | |
| const renderIssueBlock = (issueId, index) => ( | |
| issuesMap[issueId].created_at && ( | |
| <IssueBlockRoot | |
| key={issueId} | |
| issueIds={issueIds} | |
| issueId={issueId} | |
| issuesMap={issuesMap} | |
| updateIssue={updateIssue} | |
| quickActions={quickActions} | |
| canEditProperties={canEditProperties} | |
| displayProperties={displayProperties} | |
| nestingLevel={0} | |
| spacingLeft={0} | |
| containerRef={containerRef} | |
| selectionHelpers={selectionHelpers} | |
| groupId={groupId} | |
| isLastChild={index === issueIds.length - 1} | |
| isDragAllowed={isDragAllowed} | |
| canDropOverIssue={canDropOverIssue} | |
| shouldRenderByDefault={isIssueNew(issuesMap[issueId])} | |
| /> | |
| ) | |
| ); | |
| return ( | |
| <div className="relative h-full w-full"> | |
| {issueIds && issueIds.length > 0 && issueIds.map(renderIssueBlock)} | |
| </div> | |
| ); |
There was a problem hiding this comment.
@gakshita can you please make the small change requested. Apologies as i didn't notice it the first time
| ))} | ||
| issueIds.map( | ||
| (issueId: string, index: number) => | ||
| issuesMap[issueId].created_at && ( |
There was a problem hiding this comment.
@gakshita One more small change, can you please reference the issue object only inside the component. Because referencing here might rerender the entire list, when one of the issue object changes
| isLastChild={index === issueIds.length - 1} | ||
| isDragAllowed={isDragAllowed} | ||
| canDropOverIssue={canDropOverIssue} | ||
| shouldRenderByDefault={isIssueNew(issuesMap[issueId])} |
There was a problem hiding this comment.
Here as well, can you just make sure to use the logic from within the IssueBlockRoot instead.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- web/core/components/issues/issue-layouts/list/block-root.tsx (6 hunks)
Files skipped from review as they are similar to previous changes (1)
- web/core/components/issues/issue-layouts/list/block-root.tsx
rahulramesha
left a comment
rahulramesha
left a comment
There was a problem hiding this comment.
@gakshita had one minor change, please let me know once done
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- web/core/components/issues/issue-layouts/list/block-root.tsx (6 hunks)
Files skipped from review as they are similar to previous changes (1)
- web/core/components/issues/issue-layouts/list/block-root.tsx
3 participants
Summary
Flicker issue on issue creation and sub issue view fixed
[WEB-1936]
Summary by CodeRabbit
New Features
defaultValueprop for the RenderIfVisible component, allowing initial visibility state configuration.shouldRenderByDefaultprop to the IssueBlockRoot component, enhancing conditional rendering capabilities.Improvements