Feat/review detail page integation 2#1197
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| align-items: center; | ||
| justify-content: center; | ||
| bottom: 0; | ||
| bottom: -20px; |
There was a problem hiding this comment.
The change from bottom: 0; to bottom: -20px; might affect the positioning of the element. Ensure this adjustment aligns with the intended design and doesn't cause layout issues, especially if the element is meant to be visible at the bottom of its container.
| return <TableNoRecord /> | ||
| } | ||
| const { | ||
| isLoading: isDownloadingSubmission, |
There was a problem hiding this comment.
The variable isLoadingBool seems redundant since isLoading is already being used for the same purpose. Consider removing isLoadingBool to avoid confusion and redundancy.
| const { | ||
| isLoading: isLoadingProjectResult, | ||
| projectResults, | ||
| }: useFetchChallengeResultsProps = useFetchChallengeResults(props.review) |
There was a problem hiding this comment.
The useFetchChallengeResults hook is being called with props.review, but it is unclear if this is the intended behavior. Ensure that props.review is the correct argument for fetching challenge results.
| ) : props.selectedTab === 'Submission / Screening' ? ( | ||
| <TabContentScreening | ||
| screening={props.screening} | ||
| isLoadingScreening={props.isLoadingSubmission} |
There was a problem hiding this comment.
The prop isLoadingScreening is being passed as props.isLoadingSubmission, which might be misleading. Consider renaming isLoadingSubmission to isLoadingScreening for clarity if it is specifically related to screening.
| } | ||
|
|
||
| // show registrants table | ||
| return <TableRegistration datas={registrants} /> |
There was a problem hiding this comment.
The prop name datas is not conventional. Consider renaming it to data for clarity and consistency with common naming conventions.
| export const TabContentReview: FC<Props> = (props: Props) => { | ||
| const selectedTab = props.selectedTab | ||
| const reviews = props.reviews | ||
| const firstSubmissions = useMemo( |
There was a problem hiding this comment.
The useMemo hook is used here to memoize the result of maxBy function. However, if the reviews array is large, this could potentially be a performance bottleneck. Consider if memoization is necessary or if the computation can be optimized.
| () => maxBy(reviews, 'review.initialScore'), | ||
| [reviews], | ||
| ) | ||
| const { actionChallengeRole }: useRoleProps = useRole() |
There was a problem hiding this comment.
The useRole hook is used here, but it is not clear what dependencies it relies on or how it affects the component's behavior. Ensure that the hook is correctly implemented and that its dependencies are properly managed.
| return <TableNoRecord /> | ||
| } | ||
|
|
||
| return actionChallengeRole !== SUBMITTER || selectedTab === APPROVAL ? ( |
There was a problem hiding this comment.
The conditional rendering logic here could be simplified or made more readable. Consider using a more descriptive variable name or refactoring the condition to improve clarity.
|
|
||
| return ( | ||
| <TableSubmissionScreening | ||
| datas={props.screening} |
There was a problem hiding this comment.
Consider renaming the prop datas to data for clarity and consistency, as data is the more commonly used term for plural data.
|
|
||
| return ( | ||
| <TableWinners | ||
| datas={props.projectResults} |
There was a problem hiding this comment.
Consider renaming the datas prop to data for better readability and consistency with common naming conventions.
| </td> | ||
| </tr> | ||
| {!includes(WITHOUT_APPEAL, challengeInfo?.type) | ||
| && !includes(WITHOUT_APPEAL, challengeInfo?.track) |
There was a problem hiding this comment.
Consider combining the conditions using logical operators to improve readability and reduce redundancy. For example, you can use !includes(WITHOUT_APPEAL, [challengeInfo?.type, challengeInfo?.track]) to check both conditions in a single statement.
| .blockMyRoles { | ||
| display: flex; | ||
| flex-direction: column; | ||
| gap: 0; |
There was a problem hiding this comment.
Consider using a more descriptive variable name for gap instead of 0, such as gap: 0px; to improve readability and maintain consistency with other CSS properties.
|
|
||
| export const SubmissionBarInfo: FC<Props> = (props: Props) => { | ||
| const { actionChallengeRole }: useRoleProps = useRole() | ||
| const { myChallengeRoles }: useRoleProps = useRole() |
There was a problem hiding this comment.
The variable name myChallengeRoles should be checked for consistency with the rest of the codebase. Ensure that it accurately reflects the data it holds and aligns with naming conventions used elsewhere.
| icon: 'icon-handle', | ||
| title: 'My Role', | ||
| value: actionChallengeRole, | ||
| value: ( |
There was a problem hiding this comment.
Consider checking if myChallengeRoles is defined and not empty before mapping over it to prevent potential runtime errors.
| icon: 'icon-handle', | ||
| style: { | ||
| color: data.handleColor, | ||
| color: data.userInfo?.handleColor, |
There was a problem hiding this comment.
Ensure that data.userInfo is defined before accessing handleColor to avoid potential undefined errors.
| title: 'Handle', | ||
| type: 'link', | ||
| value: data.handle, | ||
| value: data.userInfo?.memberHandle, |
There was a problem hiding this comment.
Ensure that data.userInfo is defined before accessing memberHandle to avoid potential undefined errors.
| }, | ||
| ] | ||
| }, [props.submission, actionChallengeRole]) | ||
| }, [props.submission, myChallengeRoles]) |
There was a problem hiding this comment.
The dependency array for the useMemo hook has been updated. Ensure that myChallengeRoles is a necessary dependency and that its changes should trigger a re-computation of the memoized value.
| renderer: (data: ChallengeInfo) => ( | ||
| <Link to='/' onClick={bind(redirect, this, data)}> | ||
| <Link | ||
| to={`${data.id}/challenge-details`} |
There was a problem hiding this comment.
The to attribute in the Link component is being constructed using a template literal. Ensure that data.id is correctly defined and not undefined or null to prevent potential navigation errors.
| import { TableMobile } from '~/apps/admin/src/lib/components/common/TableMobile' | ||
| import { Table, TableColumn } from '~/libs/ui' | ||
| import { EnvironmentConfig } from '~/config' | ||
| import { |
There was a problem hiding this comment.
The import statement for EnvironmentConfig has been removed. Ensure that this removal does not affect any functionality that relies on environment configuration.
| } from '~/apps/admin/src/lib/hooks' | ||
|
|
||
| import { RegistrationInfo } from '../../models' | ||
| import { BackendResource } from '../../models' |
There was a problem hiding this comment.
The import for RegistrationInfo has been replaced with BackendResource. Verify that BackendResource provides all necessary properties and methods that RegistrationInfo previously offered.
| interface Props { | ||
| className?: string | ||
| datas: RegistrationInfo[] | ||
| datas: BackendResource[] |
There was a problem hiding this comment.
The type of datas has been changed from RegistrationInfo[] to BackendResource[]. Ensure that all usages of datas in the component are compatible with the new type.
| setSort, | ||
| sort, | ||
| }: useTableFilterLocalProps<RegistrationInfo> = useTableFilterLocal( | ||
| }: useTableFilterLocalProps<BackendResource> = useTableFilterLocal( |
There was a problem hiding this comment.
The type BackendResource is being used instead of RegistrationInfo. Ensure that BackendResource is the correct type for the data being handled by useTableFilterLocal. If BackendResource does not match the expected structure or fields, it could lead to runtime errors or unexpected behavior.
| const isTablet = useMemo(() => screenWidth <= 744, [screenWidth]) | ||
|
|
||
| const columns = useMemo<TableColumn<RegistrationInfo>[]>( | ||
| const columns = useMemo<TableColumn<BackendResource>[]>( |
There was a problem hiding this comment.
The type of the columns variable has been changed from TableColumn<RegistrationInfo>[] to TableColumn<BackendResource>[]. Ensure that all properties accessed within the renderer function are available in the BackendResource type.
| renderer: (data: BackendResource) => ( | ||
| <a | ||
| href={`${EnvironmentConfig.REVIEW.PROFILE_PAGE_URL}/${data.memberHandle}`} | ||
| href={getHandleUrl(data)} |
There was a problem hiding this comment.
The href attribute value has been changed from a direct URL concatenation to a function call getHandleUrl(data). Verify that the getHandleUrl function correctly constructs the URL and handles any edge cases or potential errors.
| onClick={function onClick() { | ||
| window.open( | ||
| `${EnvironmentConfig.REVIEW.PROFILE_PAGE_URL}/${data.memberHandle}`, | ||
| getHandleUrl(data), |
There was a problem hiding this comment.
Consider checking if data.memberHandle is defined before calling getHandleUrl(data) to prevent potential errors if memberHandle is undefined or null.
| label: 'Rating', | ||
| propertyName: 'rating', | ||
| renderer: (data: RegistrationInfo) => ( | ||
| renderer: (data: BackendResource) => ( |
There was a problem hiding this comment.
The type of the data parameter in the renderer function has been changed from RegistrationInfo to BackendResource. Ensure that BackendResource includes the properties handleColor and rating, as these are accessed within the function.
| label: 'Registration Date', | ||
| propertyName: 'created', | ||
| renderer: (data: RegistrationInfo) => ( | ||
| renderer: (data: BackendResource) => ( |
There was a problem hiding this comment.
The type change from RegistrationInfo to BackendResource should be verified to ensure that createdString is a valid property of BackendResource. If BackendResource does not have createdString, this could lead to runtime errors.
| ) | ||
|
|
||
| const columnsMobile = useMemo<MobileTableColumn<RegistrationInfo>[][]>( | ||
| const columnsMobile = useMemo<MobileTableColumn<BackendResource>[][]>( |
There was a problem hiding this comment.
The type BackendResource is used here instead of RegistrationInfo. Ensure that this change aligns with the intended data structure and that all related logic and data handling are correctly updated to accommodate this new type.
| mobileType: 'last-value', | ||
| }, | ||
| ] as MobileTableColumn<RegistrationInfo>[], | ||
| ] as MobileTableColumn<BackendResource>[], |
There was a problem hiding this comment.
The type cast has been changed from MobileTableColumn<RegistrationInfo>[] to MobileTableColumn<BackendResource>[]. Ensure that BackendResource is the correct type for the data being handled in this context, and verify that all properties used in the table columns are compatible with BackendResource. If BackendResource does not have the necessary properties, consider defining a new type or interface that includes all required fields.
| */ | ||
| import { FC, useCallback, useMemo } from 'react' | ||
| import { Link, NavLink } from 'react-router-dom' | ||
| import { FC, useCallback, useContext, useMemo } from 'react' |
There was a problem hiding this comment.
The useContext hook has been added, but there is no indication of how it is being used in the current diff. Ensure that useContext is necessary and properly utilized in the component.
| import { FC, useCallback, useMemo } from 'react' | ||
| import { Link, NavLink } from 'react-router-dom' | ||
| import { FC, useCallback, useContext, useMemo } from 'react' | ||
| import { Link } from 'react-router-dom' |
There was a problem hiding this comment.
The NavLink import has been removed. Verify that NavLink is not used elsewhere in the component to avoid runtime errors.
| import { useWindowSize, WindowSize } from '~/libs/shared' | ||
| import { TableMobile } from '~/apps/admin/src/lib/components/common/TableMobile' | ||
| import { Table, TableColumn } from '~/libs/ui' | ||
| import { IsRemovingType } from '~/apps/admin/src/lib/models' |
There was a problem hiding this comment.
The import statement for IsRemovingType seems unnecessary if it's not used in this file. Consider removing it to keep the code clean.
| import { ProgressBar } from '../ProgressBar' | ||
| import { ADMIN, APPROVAL, COPILOT, WITHOUT_APPEAL } from '../../../config/index.config' | ||
| import { useRole, useRoleProps } from '../../hooks' | ||
| import { getHandleUrl } from '../../utils' |
There was a problem hiding this comment.
Ensure that the getHandleUrl function is used in this file. If not, consider removing the import to avoid unused imports.
| import styles from './TableReviewAppeals.module.scss' | ||
|
|
||
| interface Props { | ||
| className?: string |
There was a problem hiding this comment.
The type property has been removed from the Props interface. Ensure that this removal is intentional and that no other parts of the codebase rely on this property.
| tab: string | ||
| type?: string | ||
| firstSubmissions?: SubmissionInfo | ||
| isDownloading: IsRemovingType |
There was a problem hiding this comment.
Consider renaming isDownloading to a more descriptive name that indicates its purpose or context, such as isSubmissionDownloading, to improve code readability.
| type?: string | ||
| firstSubmissions?: SubmissionInfo | ||
| isDownloading: IsRemovingType | ||
| downloadSubmission: (submissionId: string) => void |
There was a problem hiding this comment.
The downloadSubmission function should have error handling to manage potential failures during the download process. Consider adding a mechanism to handle errors gracefully.
| renderer: (data: SubmissionInfo) => <NavLink to='#' onClick={prevent}>{data.id}</NavLink>, | ||
| renderer: (data: SubmissionInfo) => ( | ||
| <button | ||
| onClick={function onClick() { |
There was a problem hiding this comment.
Consider using an arrow function for the onClick handler to maintain consistency with modern JavaScript practices. For example: onClick={() => props.downloadSubmission(data.id)}.
| color: data.handleColor, | ||
| color: data.userInfo?.handleColor, | ||
| }} | ||
| onClick={function onClick() { |
There was a problem hiding this comment.
The onClick handler for the anchor tag is redundant since the href attribute already opens the link in a new tab due to target='_blank'. Consider removing the onClick handler to simplify the code.
| if (includes(WITHOUT_APPEAL, props.type)) { | ||
| return [...initalColumns, ...actionColumns] as TableColumn<SubmissionInfo>[] | ||
| if ( | ||
| includes(WITHOUT_APPEAL, challengeType) |
There was a problem hiding this comment.
Consider renaming challengeType and challengeTrack to more descriptive names if they are not self-explanatory. This will improve code readability.
| return [...initalColumns, ...actionColumns] as TableColumn<SubmissionInfo>[] | ||
| if ( | ||
| includes(WITHOUT_APPEAL, challengeType) | ||
| || includes(WITHOUT_APPEAL, challengeTrack) |
There was a problem hiding this comment.
Ensure that challengeTrack is correctly defined and initialized before use. If it is a new addition, verify its source and initialization.
| [ | ||
| prevent, | ||
| props.tab, | ||
| challengeInfo, |
There was a problem hiding this comment.
It seems that challengeInfo is being added to the dependency array of the useMemo hook. Ensure that challengeInfo is a stable value or memoized itself to prevent unnecessary re-renders.
| props.tab, | ||
| challengeInfo, | ||
| actionChallengeRole, | ||
| props.isDownloading, |
There was a problem hiding this comment.
Adding props.isDownloading to the dependency array suggests that this value might change frequently. Verify that this is necessary, as frequent changes can lead to performance issues if not handled properly.
| challengeInfo, | ||
| actionChallengeRole, | ||
| props.isDownloading, | ||
| props.downloadSubmission, |
There was a problem hiding this comment.
Including props.downloadSubmission in the dependency array implies that this function might change. Ensure that downloadSubmission is either memoized or a stable reference to avoid unnecessary recalculations.
| * Table Winners. | ||
| */ | ||
| import { FC, useCallback, useMemo } from 'react' | ||
| import { FC, useCallback, useContext, useMemo } from 'react' |
There was a problem hiding this comment.
The useContext hook is added, but ensure that the ChallengeDetailContext is properly provided higher up in the component tree to avoid potential undefined errors.
| import { FC, useCallback, useContext, useMemo } from 'react' | ||
| import { Link, NavLink } from 'react-router-dom' | ||
| import _, { find, includes } from 'lodash' | ||
| import { includes, noop } from 'lodash' |
There was a problem hiding this comment.
The find function from lodash has been removed. Ensure that its removal does not affect any functionality that relied on finding specific elements in the datas array.
| import { useWindowSize, WindowSize } from '~/libs/shared' | ||
| import { TableMobile } from '~/apps/admin/src/lib/components/common/TableMobile' | ||
| import { Table, TableColumn } from '~/libs/ui' | ||
| import { IsRemovingType } from '~/apps/admin/src/lib/models' |
There was a problem hiding this comment.
The IsRemovingType import is added. Verify that this type is correctly defined and used within the component to prevent type errors.
| type?: string | ||
| tab: string | ||
| firstSubmissions?: SubmissionInfo | ||
| isDownloading: IsRemovingType |
There was a problem hiding this comment.
The isDownloading prop is added with type IsRemovingType. Ensure that this prop is correctly utilized within the component to reflect its intended functionality.
| tab: string | ||
| firstSubmissions?: SubmissionInfo | ||
| isDownloading: IsRemovingType | ||
| downloadSubmission: (submissionId: string) => void |
There was a problem hiding this comment.
The downloadSubmission function prop is added. Make sure this function is implemented and correctly handles the submission download logic.
| } | ||
|
|
||
| export const TableReviewAppealsForSubmitter: FC<Props> = (props: Props) => { | ||
| // get challenge info from challenge detail context |
There was a problem hiding this comment.
The logic for determining firstSubmission has been simplified. Ensure that the removal of the previous logic does not affect the component's functionality, especially in cases where props.tab was used to determine the firstSubmission.
| renderer: (data: SubmissionInfo) => ( | ||
| <NavLink to='#' onClick={prevent}>{data.id}</NavLink> | ||
| <button | ||
| onClick={function onClick() { |
There was a problem hiding this comment.
Consider using an arrow function for the onClick handler to maintain consistency with modern React practices and avoid potential issues with this binding.
| onClick={function onClick() { | ||
| props.downloadSubmission(data.id) | ||
| }} | ||
| className={styles.textBlue} |
There was a problem hiding this comment.
Ensure that styles.textBlue is defined and applied correctly. If the style is not defined, it could lead to unexpected UI behavior.
| props.downloadSubmission(data.id) | ||
| }} | ||
| className={styles.textBlue} | ||
| disabled={props.isDownloading[data.id]} |
There was a problem hiding this comment.
Check if props.isDownloading is properly initialized and updated to prevent potential errors when accessing data.id as a key.
| ) as ReviewItemInfo[] | ||
| const totalNumberOfQuestions = reviewItems.length | ||
| const numberOfQuestionsHaveBeenFilled = _.filter( | ||
| const numberOfQuestionsHaveBeenFilled = filter( |
There was a problem hiding this comment.
The lodash filter function has been replaced with a native filter function. Ensure that filter is correctly imported from the appropriate module or is a native JavaScript array method. If it's intended to use the native method, ensure that reviewItems is an array and the filter method is correctly applied.
| .format(TABLE_DATE_FORMAT) | ||
| : undefined | ||
| const updatedAt = new Date(data.updatedAt) | ||
| const updatedAtString = createdAt |
There was a problem hiding this comment.
The variable updatedAtString is incorrectly using createdAt for the conditional check. It should use updatedAt instead.
| ).length | ||
|
|
||
| return { | ||
| appealResuls: MockAppealResults, // use mock data |
There was a problem hiding this comment.
The appealResuls property seems to have a typo. It should be appealResults.
| finalScore: data.finalScore, | ||
| id: data.id, | ||
| initialScore: data.initialScore, | ||
| reviewItems: MockReviewEdit.reviewItems.map(adjustReviewItemInfo), |
There was a problem hiding this comment.
The reviewItems property is using MockReviewEdit.reviewItems.map(adjustReviewItemInfo), which might not reflect the actual review items from the backend. Consider using the reviewItems calculated from the submission.review for consistency with the backend data.
| / totalNumberOfQuestions, | ||
| ) | ||
| : 0, | ||
| scorecardId: '', |
There was a problem hiding this comment.
The scorecardId is being set to an empty string. Ensure this is intentional and verify if a valid ID should be assigned based on the backend data.
| ): ReviewItemInfo | undefined { | ||
| data: ReviewItemInfo, | ||
| ): ReviewItemInfo { | ||
| if (!data) { |
There was a problem hiding this comment.
The function adjustReviewItemInfo no longer accepts undefined as a valid input for the data parameter. Consider updating the function documentation to reflect this change, as the current implementation assumes data is always defined.
| : undefined | ||
|
|
||
| return { | ||
| appeals: MockAppealResults, // use mock data |
There was a problem hiding this comment.
Using mock data for appeals might not be suitable for production. Consider fetching actual data from the backend or ensure this is only temporary for testing purposes.
| appeals: MockAppealResults, // use mock data | ||
| createdAt, | ||
| createdAtString, | ||
| reviewerHandle: 'Ghostar', // use mock data |
There was a problem hiding this comment.
The reviewerHandle is set to a static value 'Ghostar'. Ensure this is replaced with dynamic data from the backend or clarify if this is intended for testing.
| createdAt, | ||
| createdAtString, | ||
| reviewerHandle: 'Ghostar', // use mock data | ||
| reviewerHandleColor: '#C9AB00', // use mock data |
There was a problem hiding this comment.
The reviewerHandleColor is hardcoded. Consider making this dynamic or configurable if it is meant to represent different reviewers.
| result: 'PASS' | 'NO PASS' | ||
| screenerId?: string | ||
| screener?: BackendResource // this field is calculated at frontend | ||
| score: string |
There was a problem hiding this comment.
The score field is currently typed as string, but it seems to be derived from data.screeningScore, which might be a number. Consider ensuring type consistency or explicitly converting data.screeningScore to a string.
| memberId: data.memberId, | ||
| result, | ||
| score: data.screeningScore ?? 'Pending', | ||
| screenerId: '', |
There was a problem hiding this comment.
The screenerId is being set to an empty string. If this is intentional, consider adding a check or comment to clarify why it defaults to an empty string. Otherwise, ensure it is being set with the correct value from the backend data.
| id: string | ||
| handle: string | ||
| handleColor: string | ||
| memberId: string |
There was a problem hiding this comment.
Consider renaming memberId to userId for consistency with common naming conventions, unless memberId is a specific term used in the domain context.
| handle: string | ||
| handleColor: string | ||
| memberId: string | ||
| userInfo?: BackendResource // this field is calculated at frontend |
There was a problem hiding this comment.
The comment // this field is calculated at frontend could be misleading. Ensure that the logic for calculating userInfo is clearly defined and documented elsewhere in the codebase or documentation.
| data.review && data.review[0] | ||
| ? convertBackendReviewToReviewInfo(data.review[0], data) | ||
| : undefined, | ||
| reviews: data.review.map(convertBackendReviewToReviewResult), |
There was a problem hiding this comment.
The map function is being used on data.review without checking if data.review is an array or if it exists. Consider adding a check to ensure data.review is an array before calling map to avoid potential runtime errors.
| /** | ||
| * Reviews service | ||
| */ | ||
| import { forEach, orderBy } from 'lodash' |
There was a problem hiding this comment.
Consider organizing imports alphabetically for better readability and consistency.
|
|
||
| import { EnvironmentConfig } from '~/config' | ||
| import { xhrGetPaginatedAsync, xhrPostAsync } from '~/libs/core' | ||
| import { |
There was a problem hiding this comment.
The import statement for xhrGetAsync and xhrGetBlobAsync seems to be added. Ensure these are necessary for the current implementation to avoid unused imports.
| const results = await xhrGetAsync< | ||
| BackendResponseWithMeta<BackendSubmission[]> | ||
| >( | ||
| `${EnvironmentConfig.REVIEW.REVIEW_API}/api/submissions?${qs.stringify({ |
There was a problem hiding this comment.
Consider handling potential errors from the xhrGetAsync call to ensure the function can gracefully handle network issues or unexpected API responses.
| submissionId: string, | ||
| ): Promise<Blob> => { | ||
| const results = await xhrGetBlobAsync<Blob>( | ||
| `${EnvironmentConfig.REVIEW.REVIEW_API}/api/submissions/${submissionId}/download`, |
There was a problem hiding this comment.
Consider adding error handling for the xhrGetBlobAsync call to manage cases where the network request might fail or return an unexpected result.
| const results = await xhrGetAsync< | ||
| BackendResponseWithMeta<BackendProjectResult[]> | ||
| >( | ||
| `${EnvironmentConfig.REVIEW.REVIEW_API}/projectResult?${qs.stringify({ |
There was a problem hiding this comment.
Consider implementing error handling for the xhrGetAsync call to manage potential network errors or unexpected API responses.
|
|
||
| const dataHavePlacement: BackendProjectResult[] = [] | ||
| const dataNoPlacement: BackendProjectResult[] = [] | ||
| forEach(results.data, item => { |
There was a problem hiding this comment.
The use of forEach here could be replaced with reduce to avoid the need for separate arrays and make the code more concise.
| sub => sub.handle === MOCKHANDLE, | ||
| ) | ||
| .map(adjustSubmissionInfo) as SubmissionInfo[] | ||
| const submission = MockSubmissions.map( |
There was a problem hiding this comment.
Filtering by sub.handle === MOCKHANDLE has been removed. If this change is intentional, ensure that it does not affect the logic where only submissions with a specific handle were previously processed. If it was removed unintentionally, consider re-adding the filter to maintain the original functionality.
| })) as SubmissionInfo[], | ||
| MockSubmissions | ||
| .map(s => { | ||
| const results = adjustSubmissionInfo(s) |
There was a problem hiding this comment.
Consider adding a semicolon at the end of the line to maintain consistency with the rest of the codebase and ensure proper syntax.
| MockSubmissions | ||
| .map(s => { | ||
| const results = adjustSubmissionInfo(s) | ||
| return ({ |
There was a problem hiding this comment.
The parentheses around the object returned from the arrow function are not necessary. You can remove them to simplify the code.
| @@ -1,5 +1,7 @@ | |||
| import { filter, some } from 'lodash' | |||
|
|
|||
| import { EnvironmentConfig } from '~/config' | |||
There was a problem hiding this comment.
It seems like EnvironmentConfig is imported but not used in this file. Consider removing the unused import to keep the code clean.
| * @returns handle url | ||
| */ | ||
| export function getHandleUrl(userInfo?: BackendResource): string { | ||
| return `${EnvironmentConfig.REVIEW.PROFILE_PAGE_URL}/${userInfo?.memberHandle}` |
There was a problem hiding this comment.
Consider adding a check to ensure userInfo and userInfo.memberHandle are defined before constructing the URL to avoid potential runtime errors.
| id: '1', | ||
| name: 'Topcoder Admin App - User Management', | ||
| submissions: MockSubmissions, | ||
| track: TRACK_CHALLENGE, |
There was a problem hiding this comment.
The variable TRACK_CHALLENGE should be defined or imported if it is not already. Ensure that TRACK_CHALLENGE is correctly declared in the scope of this file or imported from the appropriate module.
| name: 'Design Prisma Schema for Topcoder Review API', | ||
| reviewLength: 2, | ||
| submissions: [], | ||
| track: TRACK_CHALLENGE, |
There was a problem hiding this comment.
The variable TRACK_CHALLENGE should be verified to ensure it is defined and correctly imported in this context. If it is not defined in the current file, ensure it is imported from the appropriate module.
| id: '8', | ||
| name: 'Review App - UI prototype Reviewer Flow', | ||
| submissions: [], | ||
| track: TRACK_CHALLENGE, |
There was a problem hiding this comment.
The variable TRACK_CHALLENGE should be defined or imported if it is not already. Ensure that it is correctly referenced in the code.
| id: '10', | ||
| name: 'RUX - 96HR NEP Platform Design Concept Challenge', | ||
| submissions: [], | ||
| track: TRACK_CHALLENGE, |
There was a problem hiding this comment.
It seems like TRACK_CHALLENGE is being used here. Ensure that TRACK_CHALLENGE is defined and imported correctly in this file. If it's a new addition, verify its usage and consistency across the codebase.
| id: '13', | ||
| name: 'PPT to Video AI Converter Web App', | ||
| submissions: [], | ||
| track: TRACK_CHALLENGE, |
There was a problem hiding this comment.
It seems like TRACK_CHALLENGE is being used here. Ensure that TRACK_CHALLENGE is defined and imported correctly in this file to avoid any reference errors.
| id: '14', | ||
| name: 'Topcoder Admin App - Roles Management Update', | ||
| submissions: [], | ||
| track: TRACK_CHALLENGE, |
There was a problem hiding this comment.
The track property is being added here. Ensure that TRACK_CHALLENGE is defined and imported correctly in this file. If it's a new addition, verify its usage and consistency across the application.
| id: '17', | ||
| name: 'Utility to Parse and Export Challenge API Data', | ||
| submissions: [], | ||
| track: TRACK_CHALLENGE, |
There was a problem hiding this comment.
It seems like TRACK_CHALLENGE is being used here. Ensure that TRACK_CHALLENGE is defined and imported correctly in this file. If it's not defined, this could lead to a runtime error.
| handle: 'Gando19850304', | ||
| handleColor: '#616BD5', | ||
| id: '736590', | ||
| memberId: '1234', |
There was a problem hiding this comment.
The memberId value is the same for both submissions. Ensure that this is intentional and that each submission should indeed have the same memberId. If not, consider assigning unique memberId values to each submission.
| handle: 'Nikesh2003', | ||
| handleColor: '#0A0A0A', | ||
| id: '736597', | ||
| memberId: '1234', |
There was a problem hiding this comment.
Consider renaming memberId to memberID for consistency with common naming conventions where acronyms are capitalized.
| handle: 'nursoltan-s', | ||
| handleColor: '#0A0A0A', | ||
| id: '726598', | ||
| memberId: '1234', |
There was a problem hiding this comment.
Consider maintaining consistency in the naming convention. The id field is used here, while memberId is introduced. If both fields are necessary, ensure their purposes are clearly distinct. Otherwise, consider using a single identifier field.
| handle: 'stevenfrog', | ||
| handleColor: '#2D7E2D', | ||
| id: '736591', | ||
| memberId: '1234', |
There was a problem hiding this comment.
The change from handle to memberId may affect how submissions are identified. Ensure that the rest of the codebase is updated to use memberId instead of handle where necessary, and verify that this change aligns with the API integration requirements.
| handle: 'whereishammer', | ||
| handleColor: '#2D7E2D', | ||
| id: '726545', | ||
| memberId: '1234', |
There was a problem hiding this comment.
The id field has been moved up, but the handle and handleColor fields have been removed. If these fields are no longer needed, ensure that their removal does not affect other parts of the application that might rely on them.
| const { actionChallengeRole }: useRoleProps = useRole() | ||
| const params = useParams() | ||
|
|
||
| // get challenge info from challenge detail context |
There was a problem hiding this comment.
The variable params is declared but never used. Consider removing it if it's not needed.
| const { actionChallengeRole }: useRoleProps = useRole() | ||
| const params = useParams() | ||
|
|
||
| // get challenge info from challenge detail context |
There was a problem hiding this comment.
The actionChallengeRole variable is declared but not used. If it's not necessary, consider removing it to clean up the code.
| review, | ||
| reviewProgress, | ||
| screening, | ||
| }: useFetchScreeningReviewProps = useFetchScreeningReview() |
There was a problem hiding this comment.
The useFetchScreeningReview hook is called without any arguments. Ensure that this is intentional and that the hook does not require any parameters.
| <ChallengePhaseInfo challengeInfo={challengeInfo} /> | ||
| <ChallengePhaseInfo | ||
| challengeInfo={challengeInfo} | ||
| reviewProgress={reviewProgress} |
There was a problem hiding this comment.
The reviewProgress prop is being added to the ChallengePhaseInfo component. Ensure that the ChallengePhaseInfo component is updated to handle this new prop appropriately. If this prop is optional, consider providing a default value or handling undefined cases within the component.
| {reviewers.map(item => ( | ||
| <div | ||
| key={item.reviewerHandle} | ||
| key={item.id} |
There was a problem hiding this comment.
Consider using a more descriptive key if possible. Using item.id as a key is generally fine, but ensure that id is unique across all items in the reviewers array to prevent potential issues with React's reconciliation process.
| to='#' | ||
| onClick={prevent} | ||
| <a | ||
| href={getHandleUrl(item)} |
There was a problem hiding this comment.
The href attribute is being set using getHandleUrl(item). Ensure that this function returns a valid URL string to prevent potential issues with broken links.
| color: item.reviewerHandleColor, | ||
| color: item.handleColor, | ||
| }} | ||
| onClick={function onClick() { |
There was a problem hiding this comment.
The onClick function opens a new window with window.open. Since the a tag already has target='_blank', this might be redundant. Consider removing the onClick function unless it serves a specific purpose beyond opening the link in a new tab.
| const [windowSize, setWindowSize]: [WindowSize, Dispatch<SetStateAction<WindowSize>>] = useState({ | ||
| height: 0, | ||
| width: 0, | ||
| height: window.innerWidth, |
There was a problem hiding this comment.
The initial values for height and width seem to be swapped. height should be initialized with window.innerHeight and width with window.innerWidth.
2 participants
Merge the API integration for challenge: http://www.topcoder.com/challenge-details/30377630/?type=develop