PM- 1502 Scorecards filtering and listing UI #1184
Conversation
feat(PM-1503): Route modifications in scorecard
…tform-ui into PM-1505_clone-scorecard
PM-1505 - clone scorecard
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -6,18 +6,49 @@ | |||
| justify-content: flex-end; | |||
| align-items: center; | |||
| padding: 16px 0; | |||
| gap: $sp-4; | |||
| gap: $sp-2; | |||
There was a problem hiding this comment.
Consider verifying if changing the gap from $sp-4 to $sp-2 aligns with the overall design consistency and spacing guidelines of the application.
| padding: 7px 11px; | ||
|
|
||
| &:disabled { | ||
| background-color: #E9ECEF !important; |
There was a problem hiding this comment.
The use of !important can lead to specificity issues. Consider refactoring the CSS to avoid using !important if possible.
| @@ -35,4 +66,8 @@ | |||
| @media (max-width: #{$mobile-max}) { | |||
| justify-content: center; | |||
| } | |||
|
|
|||
| :global(.btn-style-secondary) { | |||
There was a problem hiding this comment.
Avoid using !important as it can make the CSS harder to maintain and override. Consider refactoring the styles to achieve the desired effect without using !important.
| @@ -84,7 +83,6 @@ const Pagination: FC<PaginationProps> = (props: PaginationProps) => { | |||
| size='md' | |||
| icon={IconOutline.ChevronLeftIcon} | |||
| iconToLeft | |||
| label='PREVIOUS' | |||
| disabled={props.page === 1 || props.disabled} | |||
There was a problem hiding this comment.
The label prop has been removed from the Button component. Ensure that this change is intentional and that the button still meets accessibility requirements, as removing text labels can affect screen readers.
| @@ -93,7 +91,6 @@ const Pagination: FC<PaginationProps> = (props: PaginationProps) => { | |||
| <Button | |||
| key={`page-${i}`} | |||
| secondary | |||
| variant='round' | |||
| label={`${i}`} | |||
| onClick={createHandlePageClick(i)} | |||
| className={i === props.page ? styles.active : ''} | |||
There was a problem hiding this comment.
The variant='round' prop has been removed from the Button component. If this was intentional, ensure that the design requirements do not require the buttons to have a round variant. If it was removed by mistake, consider adding it back to maintain the intended button style.
| @@ -107,7 +104,6 @@ const Pagination: FC<PaginationProps> = (props: PaginationProps) => { | |||
| size='md' | |||
There was a problem hiding this comment.
The label='NEXT' prop has been removed from the Button component. Ensure that this change is intentional and that the button's functionality and accessibility are not affected by the removal of the label.
| @@ -118,7 +114,6 @@ const Pagination: FC<PaginationProps> = (props: PaginationProps) => { | |||
| size='md' | |||
There was a problem hiding this comment.
The label='LAST' prop was removed from the Button component. Ensure that this change is intentional and that the UI/UX is not negatively impacted by the absence of this label.
| import { ReviewAppContext } from '../../contexts' | ||
| import { ReviewAppContextModel } from '../../models' | ||
|
|
||
| import { getTabIdFromPathName, getTabsConfig } from './config' |
There was a problem hiding this comment.
The function getTabsConfig is imported but not used in this file. Consider removing the import if it is not needed.
| const activeTabPathName: string = useMemo<string>( | ||
| () => getTabIdFromPathName(pathname), | ||
| [pathname], | ||
| () => getTabIdFromPathName(pathname, userRoles), |
There was a problem hiding this comment.
The function getTabIdFromPathName now takes an additional parameter userRoles. Ensure that this function is designed to handle the new parameter correctly and that it doesn't introduce any unintended side effects.
| ) | ||
| const [activeTab, setActiveTab]: [ | ||
| string, | ||
| Dispatch<SetStateAction<string>> | ||
| ] = useState<string>(activeTabPathName) | ||
|
|
||
| useEffect(() => { | ||
| setActiveTab(activeTabPathName) |
There was a problem hiding this comment.
The useEffect hook is used to update activeTab whenever activeTabPathName changes. Verify that this does not cause unnecessary re-renders or side effects, especially if activeTabPathName changes frequently.
| @@ -58,7 +72,7 @@ const NavTabs: FC = () => { | |||
| Review | |||
| </div> | |||
| <ul className={styles.tab}> | |||
| {TabsConfig.map(tab => ( | |||
| {tabs.map(tab => ( | |||
There was a problem hiding this comment.
The variable tabs is used here, but it is not clear from the diff if it has been defined or imported correctly. Ensure that tabs is properly defined or imported in the component to avoid runtime errors.
| @@ -5,25 +5,37 @@ import { | |||
| activeReviewAssigmentsRouteId, | |||
There was a problem hiding this comment.
The variable name activeReviewAssigmentsRouteId seems to have a typo. It should be activeReviewAssignmentsRouteId instead of activeReviewAssigmentsRouteId. Please correct the spelling.
|
|
||
| export function getTabIdFromPathName(pathname: string): string { | ||
| const matchItem = _.find(TabsConfig, item => pathname.includes(`/${item.id}`)) | ||
| if (userRoles.includes('administrator')) { |
There was a problem hiding this comment.
Consider checking for null or undefined values for userRoles before calling includes. This will prevent potential runtime errors if userRoles is not properly initialized.
| padding: 20px; | ||
| background-color: #E0E4E84D; | ||
| justify-content: center; | ||
| align-items: center; |
There was a problem hiding this comment.
The property align-items: center; is duplicated on line 8. Consider removing one of them to avoid redundancy.
| justify-content: center; | ||
| align-items: center; | ||
|
|
||
| .inputWrapper { |
There was a problem hiding this comment.
The .inputWrapper class has gap: $sp-1;, but $sp-1 is not defined in the provided code. Ensure that $sp-1 is defined in the imported styles or replace it with a valid value.
| } | ||
|
|
||
| .inputIcon { | ||
| color: $black-60; |
There was a problem hiding this comment.
The .inputIcon class uses $black-60, but this variable is not defined in the provided code. Ensure that $black-60 is defined in the imported styles or replace it with a valid value.
| font-weight: 700; | ||
| } | ||
|
|
||
| @media (max-width: 768px) { |
There was a problem hiding this comment.
The media query for max-width 768px changes the flex direction to column, which might affect the layout significantly. Ensure that this change is intended and does not disrupt the UI design.
| status: '', | ||
| type: '', | ||
| }) | ||
| }, []) |
There was a problem hiding this comment.
Consider adding props as a dependency to the useCallback hook to ensure that the latest props.onFiltersChange function is used when the component re-renders.
| } | ||
|
|
||
| .tableBreakCell { | ||
| white-space: break-spaces !important; |
There was a problem hiding this comment.
Consider using a more specific class name instead of !important to ensure styles are applied correctly without overriding other styles. This can help maintain better control over CSS specificity.
| .paginationText { | ||
| font-size: 12px; | ||
| font-weight: 400; | ||
| color: #0A0A0A; |
There was a problem hiding this comment.
The color #0A0A0A is hardcoded. Consider using a variable from your color palette for consistency and easier maintenance.
| import styles from './TableScorecards.module.scss' | ||
|
|
||
| interface Props { | ||
| className?: string |
There was a problem hiding this comment.
Consider using a more specific type instead of any for metadata to improve type safety and clarity.
| <PencilIcon /> | ||
| <span>Edit</span> | ||
| </div> | ||
| <div className={styles.actionItem} onClick={bind(props.onClone, this, data)}> |
There was a problem hiding this comment.
The use of bind here might be unnecessary. Consider using an arrow function instead to maintain the correct this context: () => props.onClone(data).
| {' '} | ||
| of | ||
| {' '} | ||
| {props.metadata.total} |
There was a problem hiding this comment.
Accessing props.metadata.total directly without checking if metadata is defined could lead to runtime errors. Consider adding a null check or default value.
|
|
||
| export interface ScorecardsResponse { | ||
| scoreCards: Scorecard[] | ||
| metadata: any |
There was a problem hiding this comment.
Consider specifying a more precise type for metadata instead of using any to improve type safety and maintainability.
| export interface ScorecardsResponse { | ||
| scoreCards: Scorecard[] | ||
| metadata: any | ||
| error?: any |
There was a problem hiding this comment.
Consider specifying a more precise type for error instead of using any to improve type safety and maintainability.
| ...(status ? { status } : {}), | ||
| }) | ||
|
|
||
| const fetcher = (url: string): Promise<ScorecardsResponse> => xhrGetAsync<ScorecardsResponse>(url) |
There was a problem hiding this comment.
Consider handling potential errors from xhrGetAsync more explicitly to ensure robust error management.
| [ProjectType.QUALITY_ASSURANCE]: 'Quality Assurance', | ||
| } | ||
|
|
||
| export const categoryByProjectType = { |
There was a problem hiding this comment.
Consider using Record<ProjectType, string[]> for categoryByProjectType to ensure type safety and consistency with other similar structures.
| 'Reporting', | ||
| 'Automated Testing', | ||
| ], | ||
| } satisfies Record<ProjectType, string[]> |
There was a problem hiding this comment.
The satisfies keyword is used here, but it might be more straightforward to use Record<ProjectType, string[]> directly in the type declaration for clarity and consistency.
| @@ -1,12 +1,26 @@ | |||
| /** | |||
| * Scorecards service | |||
| */ | |||
| import { xhrPostAsync } from '~/libs/core' | |||
There was a problem hiding this comment.
The import statement for xhrPostAsync should be organized alphabetically with the other import statements for consistency.
| import { adjustScorecardInfo, ScorecardInfo } from '../models' | ||
| import { adjustScorecardInfo, Scorecard, ScorecardInfo } from '../models' | ||
|
|
||
| const baseUrl = `${EnvironmentConfig.API.V6}/review/scorecards` |
There was a problem hiding this comment.
Consider using const for baseUrl only if it is not intended to be reassigned. Otherwise, use let.
| * @returns resolves to the cloned scorecard info | ||
| */ | ||
| export const cloneScorecard = async (scorecard: Pick<Scorecard, 'id'>): Promise<Scorecard> => ( | ||
| xhrPostAsync(`${baseUrl}/${scorecard.id}/clone`, {}) |
There was a problem hiding this comment.
Consider adding error handling for the xhrPostAsync call to manage potential failures during the cloning process. This will help ensure that any issues are properly caught and handled.
| * The router outlet. | ||
| */ | ||
|
|
||
| import { FC, PropsWithChildren, useContext, useEffect, useMemo } from 'react' |
There was a problem hiding this comment.
The import statement for Routes is missing from the react-router-dom package. Ensure that Routes is correctly imported to avoid runtime errors.
| return ( | ||
| <> | ||
| <Outlet /> | ||
| <Routes>{childRoutes}</Routes> |
There was a problem hiding this comment.
The Routes component should wrap Route components. Ensure that childRoutes returns valid Route elements to avoid rendering issues.
| function useChildRoutes(): Array<JSX.Element> | undefined { | ||
| const { getRouteElement }: RouterContextData = useContext(routerContext) | ||
| const childRoutes = useMemo( | ||
| () => reviewRoutes[0].children |
There was a problem hiding this comment.
Consider adding a null check for reviewRoutes[0] to prevent potential runtime errors if reviewRoutes is empty or undefined.
| const { getRouteElement }: RouterContextData = useContext(routerContext) | ||
| const childRoutes = useMemo( | ||
| () => reviewRoutes[0].children | ||
| ?.find(r => r.id === scorecardRouteId) |
There was a problem hiding this comment.
The optional chaining operator ?. is used, but ensure that reviewRoutes[0].children is always defined to avoid unexpected behavior.
| @@ -0,0 +1,3 @@ | |||
| .totalScorecards { | |||
| margin-top: 10px; | |||
| } | |||
There was a problem hiding this comment.
It's a good practice to ensure that files end with a newline character to avoid potential issues with some tools and version control systems.
|
|
||
| import styles from './ScorecardsListPage.module.scss' | ||
|
|
||
| export const ScorecardsListPage: FC<{}> = () => { |
There was a problem hiding this comment.
Consider specifying the type of the props object in the component definition for better type safety and clarity.
| const confirmation = useConfirmationModal() | ||
|
|
||
| const [filters, setFilters] = useState({ | ||
| category: '', |
There was a problem hiding this comment.
Consider using a more descriptive name for the filter keys, such as 'categoryFilter', 'nameFilter', etc., to improve code readability.
| scoreCards: scorecards, | ||
| metadata, | ||
| isValidating: isLoadingScorecards, | ||
| }: ScorecardsResponse = useFetchScorecards({ |
There was a problem hiding this comment.
Consider handling potential errors from the useFetchScorecards hook to ensure the application can gracefully handle API failures.
| totalPages={metadata?.totalPages} | ||
| page={page} | ||
| setPage={setPage} | ||
| datas={scorecards.map((item, i) => ({ |
There was a problem hiding this comment.
The datas prop name is unconventional. Consider renaming it to data for consistency and clarity.
| { | ||
| element: <ScorecardsListPage />, | ||
| id: 'list-scorecards-page', | ||
| route: '', |
There was a problem hiding this comment.
Consider providing a more descriptive route name instead of an empty string for better clarity and maintainability.
| @@ -29,6 +29,7 @@ const PaymentProviderTable: React.FC<PaymentMethodTableProps> = (props: PaymentM | |||
| <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 ESLint rules should be avoided unless absolutely necessary. Consider addressing the accessibility issue by ensuring that the control has an associated label, which will improve accessibility compliance.
| @@ -30,6 +30,7 @@ const TaxFormTable: React.FC<TaxFormTableProps> = (props: TaxFormTableProps) => | |||
| <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 eslint rules should be avoided unless absolutely necessary. Consider addressing the accessibility issue by ensuring that the control has an associated label, which will improve accessibility compliance.
| } | ||
|
|
||
| @media (max-width: 768px) { | ||
| .filtersContainer { |
There was a problem hiding this comment.
Consider adding a comment to explain the purpose of the flex-wrap: wrap; rule for .filtersContainer to improve readability.
| .categorySelect, | ||
| .statusSelect, | ||
| .clearButton { | ||
| flex: 1 1 calc(50% - 0.5rem); |
There was a problem hiding this comment.
The comment // Two filters per row should be removed as it violates the instruction to never suggest adding comments to the code.
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-1502
https://topcoder.atlassian.net/browse/PM-1505
What's in this PR?
Scorecards listing table and filtering UI
