Topcoder Admin App - Misc Update 0505 #1070
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -10,6 +10,7 @@ import { | |||
| useState, | |||
| } from 'react' | |||
| import { useSearchParams } from 'react-router-dom' | |||
| import _ from 'lodash' | |||
There was a problem hiding this comment.
Consider importing only the specific lodash functions you need instead of the entire library to reduce bundle size and improve performance.
| page: 1, | ||
| perPage: 25, | ||
| status: ChallengeStatus.Active, | ||
| track: null!, // eslint-disable-line @typescript-eslint/no-non-null-assertion, unicorn/no-null |
There was a problem hiding this comment.
Using null! for track may lead to runtime errors if the value is accessed before being properly initialized. Consider using a safer default value or handling the case where track is null.
| perPage: 25, | ||
| status: ChallengeStatus.Active, | ||
| track: null!, // eslint-disable-line @typescript-eslint/no-non-null-assertion, unicorn/no-null | ||
| type: null!, // eslint-disable-line @typescript-eslint/no-non-null-assertion, unicorn/no-null |
There was a problem hiding this comment.
Using null! for type may lead to runtime errors if the value is accessed before being properly initialized. Consider using a safer default value or handling the case where type is null.
| }) | ||
| const disableReset = useMemo(() => _.isEqual(filterCriteria, defaultFilter), [filterCriteria]) |
There was a problem hiding this comment.
Consider adding a dependency on defaultFilter to the useMemo hook to ensure that changes to defaultFilter are accounted for in the memoization process.
| && challenges.length === 0 | ||
| } | ||
| onReset={handleReset} | ||
| onReset={function onReset() { |
There was a problem hiding this comment.
Consider using an arrow function for onReset to maintain consistency with other inline functions in the component.
| ...defaultFilter, | ||
| }) | ||
| setTimeout(() => { | ||
| search() |
There was a problem hiding this comment.
The setTimeout function is used here without a delay argument. If the intention is to execute search() asynchronously, consider adding a comment explaining the reasoning or adjust the logic if a delay is needed.
| handleSubmit, | ||
| control, | ||
| formState: { isValid }, | ||
| formState: { isValid, isDirty }, |
There was a problem hiding this comment.
The addition of isDirty to formState is a good change for tracking form modifications, but ensure that its usage is properly integrated into the component logic to handle scenarios where the form's dirty state needs to be checked or acted upon.
| onClick={function onClick() { | ||
| reset(defaultValues) | ||
| setTimeout(() => { | ||
| onSubmit(defaultValues) |
There was a problem hiding this comment.
Consider adding a delay duration to setTimeout for clarity, even if it's 0, to explicitly indicate the intended behavior.
| @@ -19,10 +19,10 @@ import styles from './ChallengeFilters.module.scss' | |||
| interface ChallengeFiltersProps { | |||
| filterCriteria: ChallengeFilterCriteria | |||
| disabled: boolean | |||
| showResetButton: boolean | |||
| onFilterCriteriaChange: (newFilterCriteria: ChallengeFilterCriteria) => void | |||
There was a problem hiding this comment.
The property showResetButton has been removed, but there is no indication of how the reset button visibility is now controlled. Ensure that the new disableReset property is correctly managing the reset button's functionality and visibility.
| @@ -188,28 +188,24 @@ const ChallengeFilters: FC<ChallengeFiltersProps> = props => { | |||
| disabled={props.disabled} | |||
| /> | |||
| </div> | |||
| {!props.showResetButton && ( | |||
| <div className={styles.blockBtns}> | |||
There was a problem hiding this comment.
The conditional rendering logic for the showResetButton prop has been removed, which may affect the intended functionality. Consider reviewing whether this change is necessary and if it aligns with the desired behavior.
| onClick={handleReset} | ||
| disabled={props.disabled} | ||
| disabled={props.disabled || props.disableReset} |
There was a problem hiding this comment.
The disabled prop for the Reset button now includes props.disableReset. Ensure that props.disableReset is correctly set and intended to be used in conjunction with props.disabled to control the button's disabled state.
| <a | ||
| href={`${EnvironmentConfig.ADMIN.CHALLENGE_URL}/${challenge.id}`} | ||
| className={styles.challengeTitle} | ||
| onClick={function onClick() { |
There was a problem hiding this comment.
The onClick handler is redundant because the href attribute already navigates to the desired URL. Consider removing the onClick function to avoid unnecessary duplication.
| handleSubmit, | ||
| control, | ||
| formState: { isValid }, | ||
| formState: { isValid, isDirty }, |
There was a problem hiding this comment.
The addition of isDirty to formState is a good practice for tracking form changes, but ensure that its usage is consistent throughout the component to handle form reset or submission logic based on dirty state.
| <Button | ||
| secondary | ||
| onClick={function onClick() { | ||
| reset(defaultValues) |
There was a problem hiding this comment.
Consider providing a delay duration for setTimeout to ensure consistent behavior across different environments.
| }) | ||
| }} | ||
| size='lg' | ||
| disabled={!isDirty} |
There was a problem hiding this comment.
Ensure that isDirty is correctly defined and updated to avoid potential issues with the disabled state of the button.
|
|
||
| .challengeTitleLink { |
There was a problem hiding this comment.
The new class .challengeTitleLink is defined but lacks any base styles. Consider adding base styles to ensure consistent styling across different browsers and contexts.
| @@ -45,13 +46,17 @@ const ChallengeTitle: FC<{ | |||
| review: ReviewSummary | |||
| }> = props => { | |||
| const goToChallenge = useEventCallback(() => { | |||
| window.location.href = `https://www.topcoder.com/challenges/${props.review.legacyChallengeId}` | |||
| window.location.href = `${EnvironmentConfig.ADMIN.CHALLENGE_URL}/${props.review.legacyChallengeId}` | |||
There was a problem hiding this comment.
Consider adding a null or undefined check for EnvironmentConfig.ADMIN.CHALLENGE_URL to ensure it is defined before using it in the URL construction.
| return ( | ||
| <LinkButton onClick={goToChallenge} className={styles.challengeTitle}> | ||
| return props.review.legacyChallengeId ? ( | ||
| <LinkButton onClick={goToChallenge} className={styles.challengeTitleLink}> |
There was a problem hiding this comment.
The class name challengeTitleLink should be verified to ensure it is defined in the styles object. If it is a new class, make sure it is added to the corresponding CSS module.
| {props.review.challengeName} | ||
| </LinkButton> | ||
| ) : ( | ||
| <span className={styles.challengeTitleText}> |
There was a problem hiding this comment.
The class name challengeTitleText should be verified to ensure it is defined in the styles object. If it is a new class, make sure it is added to the corresponding CSS module.
| handleSubmit, | ||
| control, | ||
| formState: { errors, isValid }, | ||
| formState: { errors, isValid, isDirty }, |
There was a problem hiding this comment.
The addition of isDirty to formState is not used in the current code. Consider removing it if it's not needed, or ensure it is utilized appropriately.
| <Button | ||
| secondary | ||
| onClick={function onClick() { | ||
| reset(defaultValues) |
There was a problem hiding this comment.
Consider adding a delay parameter to setTimeout to ensure the reset operation is executed after a specific time interval. Currently, it defaults to 0, which might not be intentional.
| }) | ||
| }} | ||
| size='lg' | ||
| disabled={!isDirty} |
There was a problem hiding this comment.
Ensure that isDirty is correctly defined and updated in the component logic to accurately reflect the form's state for enabling/disabling the Reset button.
Challenge https://www.topcoder.com/challenges/852b6d05-cacd-427f-a974-99347a67801c?tab=details