- Notifications
You must be signed in to change notification settings - Fork21
PM-1904 active challenges page#1267
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| @@ -0,0 +1,5 @@ | |||
| <svgwidth="20"height="20"viewBox="0 0 20 20"fill="none"xmlns="http://www.w3.org/2000/svg"> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[accessibility]
Consider adding atitle ordesc element within the<svg> to improve accessibility for screen readers. This will help users who rely on assistive technologies understand the purpose of the icon.
| @@ -0,0 +1,10 @@ | |||
| <svgwidth="20"height="20"viewBox="0 0 20 20"fill="none"xmlns="http://www.w3.org/2000/svg"> | |||
| <gclip-path="url(#clip0_2793_151)"> | |||
| <path d="M9 12.825V16C9 16.2833 9.09583 16.5208 9.2875 16.7125C9.47917 16.9042 9.71667 17 10 17C10.2833 17 10.5208 16.9042 10.7125 16.7125C10.9042 16.5208 11 16.2833 11 16V12.825L11.9 13.725C12 13.825 12.1125 13.9 12.2375 13.95C12.3625 14 12.4875 14.0208 12.6125 14.0125C12.7375 14.0042 12.8583 13.975 12.975 13.925C13.0917 13.875 13.2 13.8 13.3 13.7C13.4833 13.5 13.5792 13.2667 13.5875 13C13.5958 12.7333 13.5 12.5 13.3 12.3L10.7 9.7C10.6 9.6 10.4917 9.52917 10.375 9.4875C10.2583 9.44583 10.1333 9.425 10 9.425C9.86667 9.425 9.74167 9.44583 9.625 9.4875C9.50833 9.52917 9.4 9.6 9.3 9.7L6.7 12.3C6.5 12.5 6.40417 12.7333 6.4125 13C6.42083 13.2667 6.525 13.5 6.725 13.7C6.925 13.8833 7.15833 13.9792 7.425 13.9875C7.69167 13.9958 7.925 13.9 8.125 13.7L9 12.825ZM4 20C3.45 20 2.97917 19.8042 2.5875 19.4125C2.19583 19.0208 2 18.55 2 18V2C2 1.45 2.19583 0.979167 2.5875 0.5875C2.97917 0.195833 3.45 0 4 0H11.175C11.4417 0 11.6958 0.05 11.9375 0.15C12.1792 0.25 12.3917 0.391667 12.575 0.575L17.425 5.425C17.6083 5.60833 17.75 5.82083 17.85 6.0625C17.95 6.30417 18 6.55833 18 6.825V18C18 18.55 17.8042 19.0208 17.4125 19.4125C17.0208 19.8042 16.55 20 16 20H4ZM11 6V2H4V18H16V7H12C11.7167 7 11.4792 6.90417 11.2875 6.7125C11.0958 6.52083 11 6.28333 11 6Z" fill="#00797A"/> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[💡maintainability]
The SVG path data is quite complex and could benefit from being split into multiple lines or using a tool to simplify it. This would improve readability and maintainability, making it easier for future developers to understand and modify the SVG if needed.
| exportconstphasesIcons={ | ||
| appeal:IconAppeal, | ||
| appealResponse:IconAppealResponse, | ||
| 'iterative review':IconReview, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[maintainability]
The key'iterative review' and'review' both map toIconReview. If these represent distinct phases, consider using different icons or clarifying the distinction to avoid confusion.
| </div> | ||
| ), | ||
| renderer:(data:ActiveReviewAssignment)=>{ | ||
| constIcon=data.hasAsAIReview ?IconAiReview :( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[correctness]
The use ofdata.currentPhase.toLowerCase() assumes thatcurrentPhase is always a valid string. Consider adding a check to ensurecurrentPhase is defined and is a string before callingtoLowerCase() to prevent potential runtime errors.
| ), | ||
| renderer:(data:ActiveReviewAssignment)=>{ | ||
| constIcon=data.hasAsAIReview ?IconAiReview :( | ||
| phasesIcons[data.currentPhase.toLowerCase()askeyoftypeofphasesIcons] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[maintainability]
The lookup inphasesIcons usingdata.currentPhase.toLowerCase() could fail silently if the key does not exist. Consider handling the case where the icon is not found to improve robustness.
| challengeEndDate:string|null | ||
| currentPhaseName:string | ||
| currentPhaseEndDate:string|null | ||
| hasAsAIReview:boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[❗❗correctness]
The propertyhasAsAIReview is added to the interfaceBackendMyReviewAssignment. Ensure that this property is correctly handled in all parts of the codebase where this interface is used, especially in serialization/deserialization logic, to prevent runtime errors or incorrect data handling.
| one or more AI reviews will be conducted for each member submission.`, | ||
| }) | ||
| return()=>notification&&removeNotification(notification.id) | ||
| },[showBannerNotification]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[❗❗correctness]
TheuseEffect dependency array should includeremoveNotification to ensure that the cleanup function always has the latest reference toremoveNotification. This prevents potential issues ifremoveNotification changes between renders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@vas3a bot right here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
kind of. removeNotification doesn't change, but I'll add it, doesn't hurt.
| <divclassName={styles.inner}> | ||
| {props.icon||( | ||
| <divclassName={styles.icon}> | ||
| <divclassName={styles.icon}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[performance]
The change from conditionally rendering theicon div to always rendering it could lead to unnecessary DOM elements whenprops.icon is not provided. Consider reverting to the previous conditional rendering approach to avoid rendering an emptydiv.
| one or more AI reviews will be conducted for each member submission.`, | ||
| }) | ||
| return()=>notification&&removeNotification(notification.id) | ||
| },[showBannerNotification]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@vas3a bot right here?
| </div> | ||
| ), | ||
| renderer:(data:ActiveReviewAssignment)=>{ | ||
| constIcon=data.hasAIReview ?IconAiReview :( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[❗❗correctness]
The propertyhasAIReview is used here, but the previous code usedhasAsAIReview. Ensure that this change is intentional and that the propertyhasAIReview exists and is correctly populated in theActiveReviewAssignment data model.
| .local() | ||
| .format(TABLE_DATE_FORMAT) | ||
| :undefined, | ||
| hasAIReview:base.hasAIReview, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[❗❗correctness]
The property name change fromhasAsAIReview tohasAIReview should be verified across the codebase to ensure consistency and avoid potential runtime errors due to mismatched property names.
| currentPhaseEndDateString?:string | ||
| challengeEndDate?:string|Date|null | ||
| challengeEndDateString?:string | ||
| hasAIReview:boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[❗❗correctness]
The variable name change fromhasAsAIReview tohasAIReview appears to be a correction. Ensure that all references to this property in the codebase are updated accordingly to prevent runtime errors.
6b054cf intofeat/ai-workflowsUh oh!
There was an error while loading.Please reload this page.
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-1904
What's in this PR?