Topcoder Admin App - Consolidate Gamification #1119
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -1,3 +1,4 @@ | |||
| import { baseDetailPath, createBadgePath } from '~/apps/gamification-admin' | |||
There was a problem hiding this comment.
The import statement for baseDetailPath and createBadgePath should be verified to ensure these paths are correct and that the modules exist in the specified location. If these are new modules, ensure they are properly implemented and tested.
| @@ -107,6 +110,17 @@ const PermissionAddGroupMembersPage: LazyLoadedComponent = lazyLoad( | |||
| 'PermissionAddGroupMembersPage', | |||
| ) | |||
|
|
|||
| const Platform: LazyLoadedComponent = lazyLoad(() => import('./platform/Platform')) | |||
There was a problem hiding this comment.
Consider renaming the variable Platform to something more descriptive, as it might be confused with a generic platform concept. A more specific name could improve code readability.
| route: `${gamificationAdminRouteId}${baseDetailPath}/:id`, | ||
| }, | ||
| ], | ||
| element: <Platform />, |
There was a problem hiding this comment.
The element property here is set to <Platform />, but it might be more appropriate to use a more descriptive component name if available, such as <GamificationPlatform />, to better reflect the purpose of this route.
|
|
||
| &.isPlatformPage { | ||
| padding: 0; | ||
| background-color: white; |
There was a problem hiding this comment.
Consider using a variable for the background-color instead of hardcoding white. This can improve maintainability and consistency across the application.
| {props.children} | ||
| return ( | ||
| <ContentLayout | ||
| innerClass={styles.contantentLayoutInner} |
There was a problem hiding this comment.
Typo in class name: contantentLayoutInner should be contentLayoutInner.
| { | ||
| children: [ | ||
| { | ||
| id: `${platformRouteId}/${gamificationAdminRouteId}`, |
There was a problem hiding this comment.
Consider using a template literal for id only if platformRouteId and gamificationAdminRouteId are guaranteed to be strings. Otherwise, ensure they are converted to strings to avoid potential runtime errors.
| () => adminRoutes[0].children | ||
| ?.find(r => r.id === platformRouteId) | ||
| ?.children?.map(getRouteElement), | ||
| [], // eslint-disable-line react-hooks/exhaustive-deps -- missing dependency: getRouteElement |
There was a problem hiding this comment.
The useMemo hook has a dependency array that is empty, which means it will only recompute the memoized value when the component mounts. If getRouteElement or any other variables used inside the useMemo callback can change, they should be included in the dependency array to ensure the memoized value is updated correctly.
| @@ -52,7 +53,7 @@ const BadgeCreatedModal: FC<BadgeCreatedModalProps> = (props: BadgeCreatedModalP | |||
| label='View' | |||
| primary | |||
| size='lg' | |||
| to={badgeDetailPath(props.badge.id)} | |||
| to={badgeDetailPath(props.rootPage, props.badge.id)} | |||
There was a problem hiding this comment.
The function badgeDetailPath now takes an additional argument props.rootPage. Ensure that this change is reflected in the definition of badgeDetailPath and that props.rootPage is always provided and valid in the context where BadgeCreatedModal is used.
| export function badgeDetailPath(badgeId: string, view?: 'edit' | 'award'): string { | ||
| return `${rootRoute}${baseDetailPath}/${badgeId}${!!view ? `#${view}` : ''}` | ||
| export function badgeDetailPath( | ||
| rootPage: string, |
There was a problem hiding this comment.
The rootPage parameter is introduced but not documented in the function signature. Ensure that the usage of rootPage is consistent and clear throughout the application.
| route: rootRoute, | ||
| }, | ||
| ] | ||
| export const createBadgeRoute: (rootPage: string) => string = ( |
There was a problem hiding this comment.
Consider providing a more descriptive name for the createBadgeRoute function to clarify its purpose, as it now takes a rootPage parameter.
| @@ -9,9 +9,10 @@ import { | |||
| RefObject, | |||
| SetStateAction, | |||
| useEffect, | |||
| useMemo, | |||
There was a problem hiding this comment.
The useMemo hook is imported but not used in the code. Consider removing it if it's not needed to avoid unnecessary imports.
| @@ -20,8 +21,6 @@ import MarkdownIt from 'markdown-it' | |||
| import sanitizeHtml from 'sanitize-html' | |||
|
|
|||
| import { | |||
There was a problem hiding this comment.
The Breadcrumb and BreadcrumbItemModel imports have been removed. Ensure that these components are not used elsewhere in the file, as their removal could lead to runtime errors if they are still needed.
| interface Props extends GameBadge { | ||
| rootPage: string; | ||
| } | ||
| const BadgeDetailPage: FC<Props> = (props: Props) => { |
There was a problem hiding this comment.
The BadgeDetailPage component now takes Props as a parameter, which extends GameBadge. Ensure that all required properties from GameBadge are being passed correctly when this component is used.
| rootPage: string; | ||
| } | ||
| const BadgeDetailPage: FC<Props> = (props: Props) => { | ||
| const initColumns = useMemo(() => badgeListingColumns(props.rootPage), [props.rootPage]) |
There was a problem hiding this comment.
The useMemo hook is used to initialize initColumns. Verify that badgeListingColumns is a pure function and does not have side effects, as useMemo should only be used for performance optimization of expensive calculations.
| @@ -106,7 +102,7 @@ const BadgeDetailPage: FC = () => { | |||
| = useState<boolean>(false) | |||
|
|
|||
| // badgeListingMutate will reset badge listing page cache when called | |||
| const sort: Sort = tableGetDefaultSort(badgeListingColumns) | |||
| const sort: Sort = tableGetDefaultSort(initColumns) | |||
There was a problem hiding this comment.
The variable initColumns should be checked to ensure it is correctly defined and initialized before being used in tableGetDefaultSort(initColumns). This will prevent potential runtime errors if initColumns is undefined or null.
| @@ -153,6 +149,7 @@ const BadgeDetailPage: FC = () => { | |||
| onBadgeUpdated() | |||
| }) | |||
| } | |||
| // eslint-disable-next-line react-hooks/exhaustive-deps | |||
There was a problem hiding this comment.
Disabling the react-hooks/exhaustive-deps rule can lead to potential issues with dependencies in the useEffect hook. Consider explicitly listing all dependencies to ensure the effect runs correctly and doesn't miss updates.
| @@ -175,6 +172,7 @@ const BadgeDetailPage: FC = () => { | |||
| default: break | |||
| } | |||
| } | |||
| // eslint-disable-next-line react-hooks/exhaustive-deps | |||
There was a problem hiding this comment.
Disabling eslint for react-hooks/exhaustive-deps can lead to potential issues with dependencies in the useEffect hook. Consider reviewing the dependencies and ensuring that all necessary dependencies are included to avoid unexpected behavior.
| @@ -201,6 +199,7 @@ const BadgeDetailPage: FC = () => { | |||
| setShowActivatedModal(true) | |||
| onBadgeUpdated() | |||
| }) | |||
| // eslint-disable-next-line no-alert | |||
There was a problem hiding this comment.
Using alert is generally discouraged in production code as it can be disruptive to the user experience. Consider using a more user-friendly notification system or logging the error instead.
| @@ -217,6 +216,7 @@ const BadgeDetailPage: FC = () => { | |||
| setShowActivatedModal(true) | |||
| onBadgeUpdated() | |||
| }) | |||
| // eslint-disable-next-line no-alert | |||
There was a problem hiding this comment.
Using alert for error handling is not recommended as it can be disruptive to the user experience. Consider using a more user-friendly way to display error messages, such as a modal or toast notification.
| secondaryButtonConfig={{ | ||
| label: 'Back', | ||
| onClick: () => { | ||
| navigate('./../../') |
There was a problem hiding this comment.
Consider using navigate(-1) instead of navigate('./../../') for going back to the previous page. This approach is more intuitive and less error-prone if the URL structure changes.
| @@ -1,4 +1,4 @@ | |||
| import { Dispatch, FC, SetStateAction, useState } from 'react' | |||
| import { Dispatch, FC, SetStateAction, useMemo, useState } from 'react' | |||
There was a problem hiding this comment.
The useMemo hook has been added to the import statement, but there is no usage of useMemo in the current diff. Ensure that useMemo is actually used in the component, or remove it from the imports to avoid unnecessary imports.
| interface Props extends GameBadge { | ||
| rootPage: string; | ||
| } | ||
| const BadgeListingPage: FC<Props> = (props: Props) => { |
There was a problem hiding this comment.
The props parameter is explicitly typed as Props, which is redundant since FC<Props> already enforces this type. Consider removing the explicit type from props.
| rootPage: string; | ||
| } | ||
| const BadgeListingPage: FC<Props> = (props: Props) => { | ||
| const initColumns = useMemo(() => badgeListingColumns(props.rootPage), [props.rootPage]) | ||
|
|
||
| const [sort, setSort]: [Sort, Dispatch<SetStateAction<Sort>>] |
There was a problem hiding this comment.
The useMemo hook is used to memoize initColumns, which is good for performance. However, ensure that badgeListingColumns is a pure function to avoid unexpected behavior.
| @@ -36,7 +40,7 @@ const BadgeListingPage: FC = () => { | |||
| // header button config | |||
| const buttonConfig: ButtonProps = { | |||
| label: 'Create New Badge', | |||
| onClick: () => navigate(createBadgeRoute), | |||
| onClick: () => navigate(createBadgeRoute(props.rootPage)), | |||
There was a problem hiding this comment.
The navigate function now takes an argument props.rootPage, but it's unclear if props is defined or passed correctly in the component. Ensure props is available and contains rootPage to avoid runtime errors.
| @@ -33,7 +37,7 @@ const BadgeActionRenderer: (badge: GameBadge) => JSX.Element | |||
| size={isMobile ? 'sm' : 'md'} | |||
| key={button.label} | |||
| label={button.label} | |||
| to={badgeDetailPath(badge.id, button.view)} | |||
| to={badgeDetailPath(props.rootPage, props.id, button.view)} | |||
There was a problem hiding this comment.
The function badgeDetailPath now takes three arguments instead of two. Ensure that props.rootPage and props.id are correctly defined and passed from the parent component. If these props are not available or incorrectly passed, it could lead to runtime errors.
| { | ||
| defaultSortDirection: 'asc', | ||
| isDefaultSort: true, | ||
| label: 'Badge Name', | ||
| propertyName: 'badge_name', | ||
| renderer: BadgeListingNameRenderer, | ||
| renderer: (data: GameBadge) => <BadgeListingNameRenderer {...data} />, |
There was a problem hiding this comment.
The change from renderer: BadgeListingNameRenderer to renderer: (data: GameBadge) => <BadgeListingNameRenderer {...data} /> introduces a new function that wraps the BadgeListingNameRenderer component. Ensure that this change is necessary and that BadgeListingNameRenderer is designed to accept props in this manner. If BadgeListingNameRenderer is a simple component that doesn't require additional props, this change might be unnecessary.
| type: 'element', | ||
| }, | ||
| { | ||
| renderer: BadgeActionRenderer, | ||
| renderer: (data: GameBadge) => <BadgeActionRenderer {...data} rootPage={rootPage} />, |
There was a problem hiding this comment.
The modification to renderer: (data: GameBadge) => <BadgeActionRenderer {...data} rootPage={rootPage} /> adds the rootPage prop to BadgeActionRenderer. Verify that BadgeActionRenderer is designed to handle the rootPage prop and that it is required for its functionality. If rootPage is not used within BadgeActionRenderer, consider removing it to avoid passing unnecessary props.
| @@ -1,21 +1,18 @@ | |||
| import { Dispatch, FC, SetStateAction, useState } from 'react' | |||
| import { Dispatch, FC, SetStateAction, useMemo, useState } from 'react' | |||
There was a problem hiding this comment.
The useMemo hook is added, but ensure that it is necessary for optimization. If createBadgeFormDef is not computationally expensive or if props.rootPage changes frequently, useMemo might not provide a significant benefit.
|
|
||
| import { Breadcrumb, BreadcrumbItemModel, ContentLayout } from '~/libs/ui' | ||
| import { ContentLayout } from '~/libs/ui' |
There was a problem hiding this comment.
The Breadcrumb and BreadcrumbItemModel imports have been removed. If breadcrumbs are no longer needed, ensure that this change aligns with the overall design and user experience requirements.
| url: '#', | ||
| }, | ||
| ]) | ||
| interface Props extends GameBadge { |
There was a problem hiding this comment.
The Props interface now extends GameBadge. Verify that all required properties from GameBadge are being correctly utilized in the component.
| interface Props extends GameBadge { | ||
| rootPage: string; | ||
| } | ||
| const CreateBadgePage: FC<Props> = (props: Props) => { |
There was a problem hiding this comment.
The CreateBadgePage component now accepts props of type Props. Ensure that all necessary properties are being passed to this component wherever it is used.
| <div className={styles.container}> | ||
| <CreateBadgeForm | ||
| formDef={createBadgeFormDef} | ||
| formDef={formDef} | ||
| onSave={onSave} |
There was a problem hiding this comment.
The variable formDef is being used here, but it is not clear from the diff if it has been defined or imported correctly. Please ensure that formDef is defined and imported properly in the file.
| onSave={onSave} | ||
| /> | ||
| </div> | ||
| { | ||
| createdBadge && ( | ||
| <BadgeCreatedModal | ||
| rootPage={props.rootPage} |
There was a problem hiding this comment.
The prop rootPage is being added to BadgeCreatedModal, but it is not clear from the diff if rootPage is a valid prop for this component. Please verify that BadgeCreatedModal accepts rootPage as a prop and that it is used correctly within the component.
| @@ -12,7 +11,7 @@ export enum CreateBadgeFormField { | |||
| file = 'file', | |||
| } | |||
|
|
|||
| export const createBadgeFormDef: FormDefinition = { | |||
| export const createBadgeFormDef: (rootPage: string) => FormDefinition = (rootPage: string) => ({ | |||
There was a problem hiding this comment.
The function signature has been changed to return a function that takes rootPage as an argument. Ensure that all usages of createBadgeFormDef are updated to pass the required rootPage parameter to avoid runtime errors.
| @@ -28,7 +27,7 @@ export const createBadgeFormDef: FormDefinition = { | |||
| { | |||
| buttonStyle: 'secondary', | |||
| icon: IconOutline.ChevronLeftIcon, | |||
| route: rootRoute || '/', | |||
| route: rootPage, | |||
There was a problem hiding this comment.
The variable rootPage is used here, but it's not clear from the diff if rootPage is defined or imported correctly. Please ensure that rootPage is defined and imported appropriately in this file or in the relevant scope.
| @@ -92,7 +92,9 @@ const TabsNavbar: FC<TabsNavbarProps> = (props: TabsNavbarProps) => { | |||
| initialTab.current = '' | |||
| } else if (props.defaultActive) { | |||
| setTabOpened(props.defaultActive) | |||
| updateOffset(props.defaultActive) | |||
| setTimeout(() => { | |||
There was a problem hiding this comment.
The use of setTimeout with a delay of 100ms to call updateOffset might introduce a potential timing issue. Consider explaining why this delay is necessary or explore alternative solutions that do not rely on arbitrary timeouts, such as using a callback or event-driven approach to ensure updateOffset is called at the correct time.
Challenge https://www.topcoder.com/challenges/699e4360-7065-4875-bdb5-5ec6be5f6d90