-
Notifications
You must be signed in to change notification settings - Fork 21
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| @@ -0,0 +1,5 @@ | |||
| <svg width="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 a title or desc 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 @@ | |||
| <svg width="20" height="20" viewBox="0 0 20 20" fill="none" xmlns="http://www.w3.org/2000/svg"> | |||
| <g clip-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.
| export const phasesIcons = { | ||
| 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 to IconReview. If these represent distinct phases, consider using different icons or clarifying the distinction to avoid confusion.
| </div> | ||
| ), | ||
| renderer: (data: ActiveReviewAssignment) => { | ||
| const Icon = 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 of data.currentPhase.toLowerCase() assumes that currentPhase is always a valid string. Consider adding a check to ensure currentPhase is defined and is a string before calling toLowerCase() to prevent potential runtime errors.
| ), | ||
| renderer: (data: ActiveReviewAssignment) => { | ||
| const Icon = data.hasAsAIReview ? IconAiReview : ( | ||
| phasesIcons[data.currentPhase.toLowerCase() as keyof typeof phasesIcons] |
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 in phasesIcons using data.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 property hasAsAIReview is added to the interface BackendMyReviewAssignment. 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]
The useEffect dependency array should include removeNotification to ensure that the cleanup function always has the latest reference to removeNotification. This prevents potential issues if removeNotification 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.
| <div className={styles.inner}> | ||
| {props.icon || ( | ||
| <div className={styles.icon}> | ||
| <div className={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 the icon div to always rendering it could lead to unnecessary DOM elements when props.icon is not provided. Consider reverting to the previous conditional rendering approach to avoid rendering an empty div.
| 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) => { | ||
| const Icon = 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 property hasAIReview is used here, but the previous code used hasAsAIReview. Ensure that this change is intentional and that the property hasAIReview exists and is correctly populated in the ActiveReviewAssignment 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 from hasAsAIReview to hasAIReview 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 from hasAsAIReview to hasAIReview appears to be a correction. Ensure that all references to this property in the codebase are updated accordingly to prevent runtime errors.
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-1904
What's in this PR?