feat(PM-1503): View scorecard implementation #1186
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| { | ||
| id, | ||
| }: UseFetchScorecardParams, | ||
| ): Scorecard { |
There was a problem hiding this comment.
The return type of the useFetchScorecard function is declared as Scorecard, but it should be Scorecard | undefined to account for the initial undefined state of the data returned by useSWR.
| fetcher, | ||
| ) | ||
|
|
||
| return data as Scorecard |
There was a problem hiding this comment.
Consider handling the case where data might be undefined before returning it. This will prevent potential runtime errors if the data is not yet loaded.
| @@ -94,4 +96,9 @@ export interface Scorecard { | |||
| category: string | |||
| status: ScorecardStatus | |||
| index?: number | |||
| version: string | |||
There was a problem hiding this comment.
Consider adding a default value or validation for the version property to ensure consistency across scorecards.
| @@ -94,4 +96,9 @@ export interface Scorecard { | |||
| category: string | |||
| status: ScorecardStatus | |||
| index?: number | |||
| version: string | |||
| challengeType: string | |||
| minScore: number | |||
There was a problem hiding this comment.
It might be beneficial to validate minScore and maxScore to ensure minScore is not greater than maxScore.
| challengeType: string | ||
| minScore: number | ||
| maxScore: number | ||
| scorecardGroups: ScorecardGroup[] |
There was a problem hiding this comment.
Ensure that scorecardGroups is properly initialized to avoid potential null or undefined errors when accessing this property.
| display: flex; | ||
| flex-direction: row; | ||
| padding: 32px; | ||
| background-color: #F6F7F9; |
There was a problem hiding this comment.
Consider using a CSS variable for the background color #F6F7F9 to maintain consistency and ease future updates.
| margin-bottom: 20px; | ||
| .label { | ||
| font-weight: 700; | ||
| font-family: "Nunito Sans", sans-serif; |
There was a problem hiding this comment.
The font-family 'Nunito Sans' is used here. Ensure that this font is loaded correctly in the application to avoid fallback to default fonts.
| font-weight: 400; | ||
| font-family: "Nunito Sans", sans-serif; | ||
| font-size: 16px; | ||
| color: #0A0A0A; |
There was a problem hiding this comment.
Consider using a CSS variable for the color #0A0A0A to maintain consistency and ease future updates.
| color: #0A0A0A; | ||
| text-transform: capitalize; | ||
| &.active { | ||
| color: #00797A; |
There was a problem hiding this comment.
Consider using a CSS variable for the color #00797A to maintain consistency and ease future updates.
| } | ||
|
|
||
| &.inactive, &.deleted { | ||
| color: #767676; |
There was a problem hiding this comment.
Consider using a CSS variable for the color #767676 to maintain consistency and ease future updates.
|
|
||
| &.inactive, &.deleted { | ||
| color: #767676; | ||
| font-family: 'Inter', sans-serif; |
There was a problem hiding this comment.
The font-family 'Inter' is used here. Ensure that this font is loaded correctly in the application to avoid fallback to default fonts.
| flex-direction: column; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Add a newline at the end of the file to follow best practices and ensure compatibility with various tools.
| } | ||
|
|
||
| const ScorecardDetails: FC<ScorecardDetailsProps> = (props: ScorecardDetailsProps) => { | ||
| const getStatusClassname = (): string => styles[ScorecardStatusLabels[props.scorecard.status]?.toLowerCase()] |
There was a problem hiding this comment.
Consider memoizing the getStatusClassname function using useCallback to prevent unnecessary re-calculations on each render.
| <div className={styles.item}> | ||
| <div className={styles.label}>Status</div> | ||
| <div | ||
| className={cn(styles.value, getStatusClassname())} |
There was a problem hiding this comment.
The cn function is used for conditional class names. Ensure that getStatusClassname() always returns a valid class name to avoid potential runtime errors.
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
It's a good practice to ensure that files end with a newline character. This can help prevent issues with certain tools and version control systems.
| <div className={styles.groupNumber}>{`Group ${index + 1}`}</div> | ||
| <div className={styles.groupInfo}> | ||
| <div className={styles.name}>{group.name}</div> | ||
| <div>{group.weight}</div> |
There was a problem hiding this comment.
Consider adding a label or unit to the group.weight value for better clarity, especially if it represents a specific metric or percentage.
| @import '@libs/ui/styles/includes'; | ||
|
|
||
| .container { | ||
| background-color: #E0E4E84D; |
There was a problem hiding this comment.
Consider using a more descriptive variable name for the color value #E0E4E84D to improve readability and maintainability.
| .section { | ||
| .heading { | ||
| padding: 12px 16px; | ||
| background-color: #00797A; |
There was a problem hiding this comment.
Consider using a more descriptive variable name for the color value #00797A to improve readability and maintainability.
| } | ||
| } | ||
| .questions { | ||
| background-color: #FFFFFF; |
There was a problem hiding this comment.
Consider using a more descriptive variable name for the color value #FFFFFF to improve readability and maintainability.
| background-color: #FFFFFF; | ||
| padding: 32px 20px; | ||
| .question { | ||
| background-color: #F6F7F9; |
There was a problem hiding this comment.
Consider using a more descriptive variable name for the color value #F6F7F9 to improve readability and maintainability.
| padding: 20px 16px; | ||
| display: flex; | ||
| flex-direction: row; | ||
| color: #0A0A0A; |
There was a problem hiding this comment.
Consider using a more descriptive variable name for the color value #0A0A0A to improve readability and maintainability.
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Add a newline at the end of the file to adhere to POSIX standards and improve compatibility with various tools.
| <div className={cn(styles.question, { | ||
| [styles.notLast]: index + 1 !== section.questions.length, | ||
| })} | ||
| > |
There was a problem hiding this comment.
Consider adding a key prop to the div element here to ensure each question has a unique key, which is important for React's reconciliation process.
| </div> | ||
| <div className={styles.detailItem}> | ||
| <div className={styles.label}>Document Upload:</div> | ||
| {/* This will be added once upload functionality works */} |
There was a problem hiding this comment.
The comment here indicates that the upload functionality is not yet implemented. Ensure that there is a plan or task to address this in the future to avoid leaving incomplete features in the codebase.
| line-height: 30px; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider adding a newline at the end of the file to adhere to POSIX standards and improve compatibility with various tools.
| const ViewScorecardPage: FC = () => { | ||
| const { scorecardId = '' }: { scorecardId?: string } = useParams<{ scorecardId: string }>() | ||
| const { profile }: ProfileContextData = useContext(profileContext) | ||
| const isAdmin = profile?.roles.includes(UserRole.administrator) |
There was a problem hiding this comment.
Consider checking if profile is defined before accessing roles to prevent potential runtime errors if profile is undefined.
| [], | ||
| ) | ||
|
|
||
| const scorecard = useFetchScorecard({ id: scorecardId as string }) |
There was a problem hiding this comment.
It might be beneficial to handle cases where useFetchScorecard returns an error or null to improve user experience and prevent rendering issues.
| <PageWrapper | ||
| pageTitle='Software General Review Scorecard' | ||
| breadCrumb={breadCrumb} | ||
| rightHeader={isAdmin && ( |
There was a problem hiding this comment.
The rightHeader prop conditionally renders the Edit Scorecard button based on isAdmin. Ensure that the Button component handles cases where iconToLeft and icon are not provided, to prevent unexpected behavior.
| @@ -57,21 +57,23 @@ export const toolTitle: string = ToolTitle.review | |||
| export const reviewRoutes: ReadonlyArray<PlatformRoute> = [ | |||
| // Review App Root | |||
| { | |||
| authRequired: true, | |||
| children: [ | |||
There was a problem hiding this comment.
The authRequired property was moved from line 60 to line 62. Ensure that this change is intentional and does not affect the route's authentication logic.
| element: <ScorecardsListPage />, | ||
| id: 'list-scorecards-page', | ||
| route: '', | ||
| }, | ||
| { | ||
| authRequired: false, |
There was a problem hiding this comment.
It might be more secure to require authentication for viewing individual scorecards. Consider setting authRequired: true for the ViewScorecardPage to ensure that only authorized users can access specific scorecard details.
| @@ -100,6 +100,7 @@ export const RouterProvider: FC<RouterProviderProps> = (props: RouterProviderPro | |||
|
|
|||
| function getRouteElement(route: PlatformRoute): JSX.Element { | |||
|
|
|||
| console.log(route, 'route') | |||
There was a problem hiding this comment.
Debugging statement console.log(route, 'route') should be removed before merging to ensure clean production code.
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-1503
What's in this PR?