Topcoder Admin App - Misc Update 0505 #1069
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.
The lodash library is imported but not used in the current code changes. Consider removing the import if it is not needed to avoid unnecessary dependencies.
| 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! with non-null assertion can lead to runtime errors if track is accessed before being properly initialized. Consider using a more type-safe approach, such as optional chaining or default values.
| 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! with non-null assertion can lead to runtime errors if type is accessed before being properly initialized. Consider using a more type-safe approach, such as optional chaining or default values.
| }) | ||
| const disableReset = useMemo(() => _.isEqual(filterCriteria, defaultFilter), [filterCriteria]) |
There was a problem hiding this comment.
Consider importing only the isEqual function from lodash instead of the entire lodash library to reduce bundle size: import isEqual from 'lodash/isEqual';
| && 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 function definitions in the component.
| ...defaultFilter, | ||
| }) | ||
| setTimeout(() => { | ||
| search() |
There was a problem hiding this comment.
The setTimeout function is used here without a delay parameter. If the intention is to execute search() immediately, consider removing setTimeout for clarity.
| handleSubmit, | ||
| control, | ||
| formState: { isValid }, | ||
| formState: { isValid, isDirty }, |
There was a problem hiding this comment.
The isDirty property is being destructured from formState but is not used anywhere in the component. Consider removing it if it's not needed.
| onClick={function onClick() { | ||
| reset(defaultValues) | ||
| setTimeout(() => { | ||
| onSubmit(defaultValues) |
There was a problem hiding this comment.
Consider providing a delay duration for setTimeout to ensure consistent behavior across different environments.
| onClick={handleReset} | ||
| disabled={props.disabled} | ||
| disabled={props.disabled || props.disableReset} |
There was a problem hiding this comment.
Consider renaming props.disableReset to props.isResetDisabled for consistency with boolean naming conventions.
| <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 setting window.location.href to the same URL as the href attribute. This is redundant and can be removed unless there is a specific reason for having both.
| handleSubmit, | ||
| control, | ||
| formState: { isValid }, | ||
| formState: { isValid, isDirty }, |
There was a problem hiding this comment.
The isDirty property is being destructured from formState but is not used anywhere in the component. If it's not needed, consider removing it to keep the code clean.
| onClick={function onClick() { | ||
| reset(defaultValues) | ||
| setTimeout(() => { | ||
| onSubmit(defaultValues) |
There was a problem hiding this comment.
Consider adding a delay duration to the setTimeout function to ensure consistent behavior. Currently, it defaults to 0 milliseconds, which might not be necessary.
|
|
||
| .challengeTitleLink { |
There was a problem hiding this comment.
The class .challengeTitleLink is defined but not used in this diff. Ensure that it is applied to the appropriate elements in the HTML/JSX to have the intended effect.
| @@ -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 verifying that EnvironmentConfig.ADMIN.CHALLENGE_URL is correctly defined and accessible to avoid potential runtime errors.
| return ( | ||
| <LinkButton onClick={goToChallenge} className={styles.challengeTitle}> | ||
| return props.review.legacyChallengeId ? ( | ||
| <LinkButton onClick={goToChallenge} className={styles.challengeTitleLink}> |
There was a problem hiding this comment.
Ensure that props.review.legacyChallengeId is always defined when expected. If it can be undefined, consider handling this case appropriately to avoid unexpected behavior.
| return ( | ||
| <LinkButton onClick={goToChallenge} className={styles.challengeTitle}> | ||
| return props.review.legacyChallengeId ? ( | ||
| <LinkButton onClick={goToChallenge} className={styles.challengeTitleLink}> | ||
| {props.review.challengeName} |
There was a problem hiding this comment.
The class name styles.challengeTitleLink should be verified to ensure it matches the intended styling. If this is a new class name, ensure it is defined in the styles.
| {props.review.challengeName} | ||
| </LinkButton> | ||
| ) : ( | ||
| <span className={styles.challengeTitleText}> |
There was a problem hiding this comment.
The class name styles.challengeTitleText should be verified to ensure it matches the intended styling. If this is a new class name, ensure it is defined in the styles.
| @@ -44,3 +44,9 @@ | |||
| font-size: 14px; | |||
| color: $black-60; | |||
| } | |||
|
|
|||
| .blockBtns { | |||
There was a problem hiding this comment.
Consider using a more descriptive class name than .blockBtns to improve readability and maintainability of the code. A name that reflects the purpose or context of the buttons would be beneficial.
| handleSubmit, | ||
| control, | ||
| formState: { errors, isValid }, | ||
| formState: { errors, isValid, isDirty }, |
There was a problem hiding this comment.
The addition of isDirty to the destructured formState object is a good practice for tracking form changes. However, ensure that isDirty is actually used in the component logic to avoid unnecessary destructuring.
| <Button | ||
| secondary | ||
| onClick={function onClick() { | ||
| reset(defaultValues) |
There was a problem hiding this comment.
Consider adding a delay duration to the setTimeout function to ensure consistent behavior across different environments.
| }) | ||
| }} | ||
| size='lg' | ||
| disabled={!isDirty} |
There was a problem hiding this comment.
The disabled attribute is set based on !isDirty. Ensure that isDirty is correctly updated to reflect the form's state, as this affects the usability of the Reset button.
Challenge https://www.topcoder.com/challenges/852b6d05-cacd-427f-a974-99347a67801c?tab=details