feat(PM-1503): Route modifications in scorecard #1183
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| * The router outlet. | ||
| */ | ||
|
|
||
| import { FC, PropsWithChildren, useContext, useEffect, useMemo } from 'react' |
There was a problem hiding this comment.
Consider removing unused import PropsWithChildren since ScorecardsContainer does not utilize any children props.
|
|
||
| return ( | ||
| <> | ||
| <Outlet /> |
There was a problem hiding this comment.
The <Outlet /> component is typically used to render child routes. Ensure that the routing logic correctly supports this use case, as it might be redundant with the <Routes>{childRoutes}</Routes> rendering.
| @@ -1,10 +1,10 @@ | |||
| import { FC, useCallback, useMemo, useState } from 'react' | |||
| import { FC, useCallback, useContext, useMemo, useState } from 'react' | |||
There was a problem hiding this comment.
It seems like useContext has been added to the import statement, but there is no usage of useContext in the current code diff. Ensure that useContext is actually needed; otherwise, it should be removed to avoid unnecessary imports.
|
|
||
| import { PageWrapper, ScorecardsFilter, TableNoRecord, TableScorecards } from '../../lib' | ||
| import { ScorecardsResponse, useFetchScorecards } from '../../lib/hooks' | ||
| import { PageWrapper, ScorecardsFilter, TableNoRecord, TableScorecards } from '../../../lib' |
There was a problem hiding this comment.
The import path for PageWrapper, ScorecardsFilter, TableNoRecord, and TableScorecards has been changed from ../../lib to ../../../lib. Verify that this change is correct and that the new path accurately reflects the location of these modules.
| import { PageWrapper, ScorecardsFilter, TableNoRecord, TableScorecards } from '../../lib' | ||
| import { ScorecardsResponse, useFetchScorecards } from '../../lib/hooks' | ||
| import { PageWrapper, ScorecardsFilter, TableNoRecord, TableScorecards } from '../../../lib' | ||
| import { ScorecardsResponse, useFetchScorecards } from '../../../lib/hooks' |
There was a problem hiding this comment.
The import path for ScorecardsResponse and useFetchScorecards has been changed from ../../lib/hooks to ../../../lib/hooks. Ensure that this change is correct and that the new path accurately reflects the location of these hooks.
| }, | ||
| { | ||
| element: <ScorecardsListPage />, | ||
| element: <ScorecardsContainer />, |
There was a problem hiding this comment.
The ScorecardsContainer component is introduced here, but ensure that it properly handles the routing logic for its children components ScorecardsListPage and ViewScorecardPage. Verify that the container is necessary and that it doesn't introduce unnecessary complexity.
| { | ||
| element: <ViewScorecardPage />, | ||
| id: 'view-scorecard-page', | ||
| route: ':scorecardId', |
There was a problem hiding this comment.
The route :scorecardId suggests dynamic routing based on scorecard IDs. Ensure that the ViewScorecardPage component is equipped to handle cases where the scorecardId might be invalid or missing, and consider implementing error handling or redirection logic.
|
|
||
| const ViewScorecardPage: FC = () => ( | ||
| <div>View scorecard</div> | ||
| ) |
There was a problem hiding this comment.
Consider adding a semicolon at the end of the line for consistency with the rest of the codebase.
| }, | ||
|
|
||
| ], | ||
| element: <ScorecardsContainer />, |
There was a problem hiding this comment.
The element property is added here, but ensure that <ScorecardsContainer /> is correctly imported and defined in the file. If it is not imported, the application will fail to render this component.
|
|
||
| ], | ||
| element: <ScorecardsContainer />, | ||
| id: scorecardRouteId, |
There was a problem hiding this comment.
The id and route properties are set to scorecardRouteId. Verify that scorecardRouteId is defined and correctly imported in this file. If it is not defined, it will cause errors in routing.
| <th className='body-ultra-small-bold'>CONNECTED PROVIDER</th> | ||
| <th className='body-ultra-small-bold'>PROVIDER ID</th> | ||
| <th className='body-ultra-small-bold'>STATUS</th> | ||
| {/* eslint-disable-next-line jsx-a11y/control-has-associated-label */} |
There was a problem hiding this comment.
Disabling the eslint rule jsx-a11y/control-has-associated-label might lead to accessibility issues. Consider providing an appropriate label for the control to ensure it is accessible to all users.
| <th className='body-ultra-small-bold'>FORM</th> | ||
| <th className='body-ultra-small-bold'>DATE FILED</th> | ||
| <th className='body-ultra-small-bold'>STATUS</th> | ||
| {/* eslint-disable-next-line jsx-a11y/control-has-associated-label */} |
There was a problem hiding this comment.
Disabling the eslint rule jsx-a11y/control-has-associated-label may lead to accessibility issues. Consider providing an associated label for the control to ensure it is accessible to all users.
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-1503
What's in this PR?