Consolidate gamification, skills manager, add SSO users option in system-admin app #1122
Conversation
Topcoder Admin App - Consolidate Gamification
Conflicts: src/apps/admin/src/admin-app.routes.tsx src/apps/admin/src/config/routes.config.ts src/apps/admin/src/lib/components/common/Layout/Layout.tsx src/apps/admin/src/lib/components/common/Tab/config/system-admin-tabs-config.ts
Topcoder Admin App - Add SSOLogin to User Management
Consolidate Skills Manager
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -3,7 +3,7 @@ import { Outlet, Routes } from 'react-router-dom' | |||
|
|
|||
| import { routerContext, RouterContextData } from '~/libs/core' | |||
|
|
|||
| import { AdminAppContextProvider, Layout, SWRConfigProvider } from './lib' | |||
| import { AdminAppContextProvider, LayoutProps, SWRConfigProvider, useLayout } from './lib' | |||
There was a problem hiding this comment.
The LayoutProps import is added but not used in the code. Consider removing it if it's unnecessary to avoid cluttering the imports.
| @@ -14,6 +14,7 @@ const AdminApp: FC = () => { | |||
| const { getChildRoutes }: RouterContextData = useContext(routerContext) | |||
| // eslint-disable-next-line react-hooks/exhaustive-deps -- missing dependency: getChildRoutes | |||
| const childRoutes = useMemo(() => getChildRoutes(toolTitle), []) | |||
| const { Layout }: { Layout: FC<LayoutProps> } = useLayout() | |||
There was a problem hiding this comment.
The destructuring of Layout from useLayout() assumes that useLayout() always returns an object with a Layout property. Consider adding error handling or validation to ensure Layout is defined before using it.
| { | ||
| element: <SkillManagementLandingPage />, | ||
| id: 'skills-landing-page', | ||
| route: '', |
There was a problem hiding this comment.
Consider adding a more descriptive route path instead of an empty string for better clarity and maintainability.
| rootPage={`${rootRoute}/${platformRouteId}/${gamificationAdminRouteId}`} | ||
| /> | ||
| ), | ||
| route: gamificationAdminRouteId, |
There was a problem hiding this comment.
Ensure that the gamificationAdminRouteId is correctly defined and used consistently across the application to avoid potential routing issues.
| }, | ||
| { | ||
| element: <CreateBadgePage />, | ||
| route: `${gamificationAdminRouteId}${createBadgePath}`, |
There was a problem hiding this comment.
Verify that createBadgePath is correctly defined and does not conflict with other routes to prevent navigation errors.
| }, | ||
| { | ||
| element: <BadgeDetailPage />, | ||
| route: `${gamificationAdminRouteId}${baseDetailPath}/:id`, |
There was a problem hiding this comment.
Check that baseDetailPath is correctly defined and that the :id parameter is handled appropriately in the BadgeDetailPage component to ensure proper functionality.
| @@ -67,7 +67,7 @@ export const DialogEditUserEmail: FC<Props> = (props: Props) => { | |||
| className={classNames(styles.container, props.className)} | |||
| onSubmit={handleSubmit(onSubmit)} | |||
| > | |||
| <div> | |||
| <div className={styles.blockForm}> | |||
There was a problem hiding this comment.
Consider verifying if styles.blockForm is defined in the styles module to avoid potential runtime errors if it is undefined.
| @@ -0,0 +1,71 @@ | |||
| .modal { | |||
| width: 800px !important; | |||
There was a problem hiding this comment.
Avoid using !important as it can make the CSS harder to maintain and override. Consider refactoring the CSS to achieve the desired styling without !important.
| align-items: flex-start; | ||
|
|
||
| th:first-child { | ||
| padding-left: 16px !important; |
There was a problem hiding this comment.
Avoid using !important for padding. Consider restructuring the CSS to achieve the desired effect without !important.
| } | ||
|
|
||
| .tableCell { | ||
| white-space: break-spaces !important; |
There was a problem hiding this comment.
Avoid using !important for white-space. Consider restructuring the CSS to achieve the desired effect without !important.
|
|
||
| .tableCell { | ||
| white-space: break-spaces !important; | ||
| text-align: left !important; |
There was a problem hiding this comment.
Avoid using !important for text-align. Consider restructuring the CSS to achieve the desired effect without !important.
| width: 100%; | ||
| } | ||
|
|
||
| .btnActions { |
There was a problem hiding this comment.
The class .btnActions appears to have the same styles as .actionButtons. Consider consolidating these classes if they are intended to be identical.
| export const DialogEditUserSSOLogin: FC<Props> = (props: Props) => { | ||
| const { width: screenWidth }: WindowSize = useWindowSize() | ||
| const isTablet = useMemo(() => screenWidth <= 900, [screenWidth]) | ||
| const { |
There was a problem hiding this comment.
Consider renaming isTablet to something more descriptive like isScreenWidthTabletOrSmaller to improve readability and clarity about what the condition represents.
| doRemoveSSOUserLogin, | ||
| }: useManageUserSSOLoginProps = useManageUserSSOLogin(props.userInfo) | ||
| const isRemovingBool = useMemo( | ||
| () => _.some(isRemoving, value => value === true), |
There was a problem hiding this comment.
The use of _.some here is appropriate, but consider using native JavaScript methods like Array.prototype.some for consistency and to avoid unnecessary dependency on lodash.
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [isLoading]) | ||
| const columns = useMemo<TableColumn<SSOUserLogin>[]>( |
There was a problem hiding this comment.
The columns array is defined using useMemo, but it depends on doRemoveSSOUserLogin, which is a function. Ensure that doRemoveSSOUserLogin is stable or memoized to prevent unnecessary re-renders.
| ) | ||
|
|
||
| const columnsMobile = useMemo<MobileTableColumn<SSOUserLogin>[][]>( | ||
| () => columns.map(column => { |
There was a problem hiding this comment.
The columnsMobile array is defined using useMemo, but it depends on columns, which is another memoized value. Ensure that columns is stable or memoized to prevent unnecessary re-renders.
|
|
||
| return ( | ||
| <BaseModal | ||
| allowBodyScroll |
There was a problem hiding this comment.
The allowBodyScroll and blockScroll props on BaseModal seem contradictory. Verify if both are needed or if one should be removed.
| columns={columns} | ||
| data={ssoUserLogins} | ||
| disableSorting | ||
| onToggleSort={_.noop} |
There was a problem hiding this comment.
The onToggleSort prop is set to _.noop, which is fine, but ensure that this is the intended behavior and that sorting is not required for this table.
| userId: props.editingData.userId, | ||
| }) | ||
| } | ||
| }, [props.editingData]) |
There was a problem hiding this comment.
Consider adding reset to the dependency array to avoid potential stale closure issues.
| inputControl={register('userId')} | ||
| dirty | ||
| disabled={props.isAdding} | ||
| /> |
There was a problem hiding this comment.
The forceUpdateValue and onChange={_.noop} props in InputText components might be unnecessary if the input is disabled. Consider removing them to simplify the code.
| error={_.get(errors, 'name.message')} | ||
| inputControl={register('name')} | ||
| dirty | ||
| disabled={props.isAdding} |
There was a problem hiding this comment.
The forceUpdateValue and onChange={_.noop} props in InputText components might be unnecessary if the input is disabled. Consider removing them to simplify the code.
| error={_.get(errors, 'email.message')} | ||
| inputControl={register('email')} | ||
| dirty | ||
| disabled={props.isAdding} |
There was a problem hiding this comment.
The forceUpdateValue and onChange={_.noop} props in InputText components might be unnecessary if the input is disabled. Consider removing them to simplify the code.
| import { DialogEditUserTerms } from '../DialogEditUserTerms' | ||
| import { DialogEditUserStatus } from '../DialogEditUserStatus' | ||
| import { DialogUserStatusHistory } from '../DialogUserStatusHistory' | ||
| import { DropdownMenuButton } from '../common/DropdownMenuButton' | ||
| import { useTableFilterLocal, useTableFilterLocalProps } from '../../hooks' | ||
| import { useOnComponentDidMount, useTableFilterLocal, useTableFilterLocalProps } from '../../hooks' |
There was a problem hiding this comment.
Consider checking if useOnComponentDidMount is actually used in the component. If it's imported but not used, it should be removed to avoid unnecessary imports.
| import { Pagination } from '../common/Pagination' | ||
| import { ReactComponent as RectangleListRegularIcon } from '../../assets/i/rectangle-list-regular-icon.svg' | ||
| import { fetchSSOLoginProviders } from '../../services' |
There was a problem hiding this comment.
Ensure that fetchSSOLoginProviders is used within the component. If it's imported but not utilized, it should be removed to maintain clean code.
| import { Pagination } from '../common/Pagination' | ||
| import { ReactComponent as RectangleListRegularIcon } from '../../assets/i/rectangle-list-regular-icon.svg' | ||
| import { fetchSSOLoginProviders } from '../../services' | ||
| import { handleError } from '../../utils' |
There was a problem hiding this comment.
Verify that handleError is implemented in the component logic. If it's not used, consider removing the import to reduce clutter.
| @@ -56,6 +60,9 @@ export const UsersTable: FC<Props> = props => { | |||
| const [showDialogEditUserGroups, setShowDialogEditUserGroups] = useState< | |||
| UserInfo | undefined | |||
| >() | |||
| const [showDialogEditUserSSOLogin, setShowDialogEditSSOLogin] = useState< | |||
There was a problem hiding this comment.
The variable name showDialogEditUserSSOLogin could be more concise. Consider renaming it to showEditSSOLoginDialog for consistency with other state variables.
| @@ -359,6 +370,16 @@ export const UsersTable: FC<Props> = props => { | |||
| [isTablet, isMobile], | |||
| ) | |||
|
|
|||
| useOnComponentDidMount(() => { | |||
There was a problem hiding this comment.
Consider adding a dependency array to useOnComponentDidMount to ensure that the effect is only run once when the component mounts. This can help prevent potential issues with re-renders.
| setSsoLoginProviders(result) | ||
| }) | ||
| .catch(e => { | ||
| handleError(e) |
There was a problem hiding this comment.
Ensure that handleError is defined and handles errors appropriately. Consider logging the error or providing user feedback if necessary.
| {showDialogEditUserSSOLogin && ( | ||
| <DialogEditUserSSOLogin | ||
| open | ||
| setOpen={function setOpen() { |
There was a problem hiding this comment.
Consider using an arrow function for consistency with other function definitions in the codebase.
|
|
||
| &.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 will make it easier to maintain and update the color scheme in the future.
|
|
||
| @include ltelg { | ||
| padding: 0; | ||
| } |
There was a problem hiding this comment.
Typo in class name: .contantentLayoutInner should be corrected to .contentLayoutInner.
| ) | ||
| export const NullLayout: FC<PropsWithChildren> = props => <>{props.children}</> | ||
|
|
||
| export type LayoutProps = PropsWithChildren<{ |
There was a problem hiding this comment.
The comment // eslint-disable-line react/no-unused-prop-types -- it's actually used suggests that the classes prop is used, but it might be better to verify if the ESLint rule can be configured to recognize its usage instead of disabling it inline.
| export function useLayout(): { Layout: FC<LayoutProps> } { | ||
| const routerContextData: RouterContextData = useContext(routerContext) | ||
|
|
||
| if (!routerContextData.initialized) return { Layout } |
There was a problem hiding this comment.
Consider adding error handling for the case where routerContextData might not be initialized properly, to prevent potential runtime errors.
| ? `/${platformRouteId}/${platformSkillRouteId}` | ||
| : `/${AppSubdomain.admin}/${platformRouteId}/${platformSkillRouteId}` | ||
|
|
||
| if (window.location.pathname.toLowerCase() |
There was a problem hiding this comment.
The use of window.location.pathname.toLowerCase() could be extracted into a utility function to improve readability and maintainability, especially since it's used multiple times.
| { | ||
| children: [ | ||
| { | ||
|
|
There was a problem hiding this comment.
Remove the extra newline at the beginning of this block to maintain consistent formatting.
| { | ||
| id: `${platformRouteId}/${gamificationAdminRouteId}`, | ||
| title: 'Badges', | ||
|
|
There was a problem hiding this comment.
Remove the extra newline at the end of this block to maintain consistent formatting.
| case SSOUserLoginsActionType.UPDATE_SSO_USER_LOGIN_INIT: { | ||
| return { | ||
| ...previousState, | ||
| isAdding: true, |
There was a problem hiding this comment.
The isAdding state is being used for both adding and updating SSO user logins. Consider using a separate state for updating to avoid potential confusion and ensure clarity in the reducer logic.
| ...previousState, | ||
| isAdding: false, | ||
| ssoUserLogins: previousState.ssoUserLogins.map( | ||
| item => (item.provider === action.payload.provider |
There was a problem hiding this comment.
The comparison item.provider === action.payload.provider assumes that provider is a unique identifier. Ensure that this assumption holds true, or consider using a more reliable unique identifier for comparison.
| } | ||
|
|
||
| case SSOUserLoginsActionType.REMOVE_SSO_USER_LOGIN_DONE: { | ||
| const ssoUserLogins = _.filter( |
There was a problem hiding this comment.
The use of _.filter from lodash is correct, but since ES6, native array methods like filter are available and might be preferable for readability and performance unless lodash's specific features are needed.
| }) | ||
|
|
||
| const doFetchSSOUserLogins = useCallback(() => { | ||
| dispatch({ |
There was a problem hiding this comment.
Consider adding error handling for the Promise.all call to handle cases where one of the promises fails, but not the other. This will ensure that partial data does not cause unexpected behavior.
| type: SSOUserLoginsActionType.ADD_SSO_USER_LOGIN_INIT, | ||
| }) | ||
| createSSOUserLogin(userInfo.id, formData) | ||
| .then(result => { |
There was a problem hiding this comment.
The toast.success call is used here to notify the user of a successful operation. Ensure that the toastId is unique across different operations to prevent potential issues with toast notifications.
| email: string | ||
| providerType: string | ||
| provider: string | ||
| context: any |
There was a problem hiding this comment.
Consider specifying a more precise type for context instead of using any to improve type safety and maintainability.
| /** | ||
| * Fetch list of sso user login. | ||
| * @param userId user id. | ||
| * @returns resolves to sso user logins |
There was a problem hiding this comment.
Consider renaming the function to fetchSSOUserLoginsByUserId to make it more descriptive and consistent with the parameter it takes.
| } | ||
|
|
||
| /** | ||
| * Fetch list of sso login provider. |
There was a problem hiding this comment.
The return description in the JSDoc comment should be updated to reflect that it resolves to SSO login providers, not user logins.
| * Create sso user login. | ||
| * @param userId user id. | ||
| * @param userLogin user login info. | ||
| * @returns resolves to sso user login |
There was a problem hiding this comment.
Consider updating the JSDoc comment to specify that it resolves to the created SSO user login.
| * Update sso user login. | ||
| * @param userId user id. | ||
| * @param userLogin user login info. | ||
| * @returns resolves to sso user login |
There was a problem hiding this comment.
Consider updating the JSDoc comment to specify that it resolves to the updated SSO user login.
| * Delete sso user login. | ||
| * @param userId user id. | ||
| * @param provider login provider. | ||
| * @returns resolves to sso user login |
There was a problem hiding this comment.
Consider updating the JSDoc comment to specify that it resolves to the deleted SSO user login.
| () => 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.
Using // eslint-disable-line react-hooks/exhaustive-deps to suppress the missing dependency warning might lead to potential issues if getRouteElement changes. Consider adding getRouteElement to the dependency array to ensure the memoization is updated correctly when it changes.
| () => 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.
Using useMemo without dependencies can lead to stale values if adminRoutes or platformRouteId change. Consider adding these as dependencies to ensure childRoutes is recalculated when necessary.
| @@ -1,17 +1,17 @@ | |||
| import { FC } from 'react' | |||
|
|
|||
| import { ContentLayout, InputCheckbox, PageTitle } from '~/libs/ui' | |||
| import { InputCheckbox, PageTitle } from '~/libs/ui' | |||
There was a problem hiding this comment.
The import for ContentLayout has been removed. Ensure that this component is no longer needed in the file. If it is still required, re-add the import statement.
|
|
||
| import { SkillsManagerContextValue, useSkillsManagerContext } from '../context' | ||
| import { SkillsManagerContextValue, useSkillsManagerContext } from '../lib/context' |
There was a problem hiding this comment.
The import path for useSkillsManagerContext has been changed from ../context to ../lib/context. Verify that this new path is correct and that the module is accessible from this location.
| @@ -30,7 +30,7 @@ const LandingPage: FC<{}> = () => { | |||
| }: SkillsManagerContextValue = useSkillsManagerContext() | |||
|
|
|||
| return ( | |||
| <ContentLayout innerClass={styles.contentWrap}> | |||
| <div className={styles.contentWrap}> | |||
There was a problem hiding this comment.
Consider using a more semantic HTML element instead of a <div> if possible, to improve accessibility and maintain semantic structure.
| @@ -63,7 +63,7 @@ const LandingPage: FC<{}> = () => { | |||
| )} | |||
|
|
|||
| {!!showSkillModal && <SkillModal skill={showSkillModal} />} | |||
| </ContentLayout> | |||
| </div> | |||
There was a problem hiding this comment.
The closing tag for ContentLayout has been changed to a div. Ensure that this change is intentional and that the surrounding layout and styling are not adversely affected by this modification.
| ?.find(r => r.id === platformRouteId) | ||
| ?.children?.find(r => r.id === platformSkillRouteId) | ||
| ?.children?.map(getRouteElement), | ||
| [], // eslint-disable-line react-hooks/exhaustive-deps -- missing dependency: getRouteElement |
There was a problem hiding this comment.
Using useMemo without dependencies can lead to unexpected behavior. Consider adding getRouteElement as a dependency to ensure that the memoization is updated when getRouteElement changes.
| @@ -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 parameter props.rootPage. Ensure that badgeDetailPath is correctly handling this new parameter and that props.rootPage is always defined when this component 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 added to the badgeDetailPath function, but there is no explanation or context in the code for why this change was necessary. Ensure that the calling code provides the correct rootPage value.
| route: rootRoute, | ||
| }, | ||
| ] | ||
| export const createBadgeRoute: (rootPage: string) => string = ( |
There was a problem hiding this comment.
The createBadgeRoute function now takes a rootPage parameter, which changes its usage. Ensure that all calls to createBadgeRoute are updated to pass the appropriate rootPage value.
| @@ -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.
| useState, | ||
| } from 'react' | ||
| import { Params, useLocation, useParams } from 'react-router-dom' | ||
| import { Params, useLocation, useNavigate, useParams } from 'react-router-dom' |
There was a problem hiding this comment.
The useNavigate 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 no longer needed in this file. If they are still required elsewhere, make sure they are imported correctly.
| 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 correctly utilized within the component.
| 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 with badgeListingColumns(props.rootPage). Verify that badgeListingColumns correctly handles the rootPage parameter and that props.rootPage is a valid dependency for this memoization.
| @@ -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 is used here, but it's not clear from the diff if initColumns is defined or imported correctly. Ensure that initColumns is properly defined or imported in the file to avoid runtime errors.
| @@ -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 ensuring that all necessary dependencies are included in the dependency array to avoid unexpected behavior.
| @@ -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 the react-hooks/exhaustive-deps rule can lead to potential issues with dependencies in the useEffect hook. Consider ensuring that all necessary dependencies are included in the dependency array 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) to go back to the previous page in the history stack instead of a relative path. This can be more reliable if the component is used in different routes.
| @@ -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 it is not used anywhere in the code. Consider removing it if it is not needed to avoid unnecessary imports.
| interface Props extends GameBadge { | ||
| rootPage: string; | ||
| } | ||
| const BadgeListingPage: FC<Props> = (props: Props) => { |
There was a problem hiding this comment.
Suggestion
Consider renaming the props parameter to { rootPage } for destructuring directly in the function signature. This can make the code cleaner and more readable.
const BadgeListingPage: FC<Props> = ({ rootPage }) => {This change will also require updating props.rootPage to rootPage in the useMemo hook.
| 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.
Suggestion
The useState hook for sort is using tableGetDefaultSort(initColumns). Ensure that tableGetDefaultSort is correctly handling the updated initColumns and that it doesn't rely on the previous badgeListingColumns directly.
| @@ -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 not clear if props is defined or passed correctly. Ensure that props is available in the component and contains rootPage. If props is not defined, this will throw an error.
| const BadgeActionRenderer: (badge: GameBadge) => JSX.Element | ||
| = (badge: GameBadge): JSX.Element => { | ||
| interface Props extends GameBadge { | ||
| rootPage: string; |
There was a problem hiding this comment.
Consider renaming the rootPage property to something more descriptive if it represents a specific concept or functionality. This will improve code readability and maintainability.
| } | ||
|
|
||
| const BadgeActionRenderer: (props: Props) => JSX.Element | ||
| = (props: Props): JSX.Element => { | ||
|
|
||
| const isMobile: boolean = useCheckIsMobile() |
There was a problem hiding this comment.
The isMobile variable is declared but not used in the provided code snippet. Ensure that it is utilized if necessary, or remove it if it is not needed.
| @@ -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 props.rootPage and props.id as arguments instead of badge.id. Ensure that props.rootPage and props.id are correctly defined and passed to this component. If they are not, this change might break the navigation logic.
| @@ -5,17 +5,17 @@ import { GameBadge } from '../../../game-lib' | |||
| import { BadgeActionRenderer } from './badge-action-renderer' | |||
| import { BadgeListingNameRenderer } from './badge-name-renderer' | |||
|
|
|||
| export const badgeListingColumns: ReadonlyArray<TableColumn<GameBadge>> = [ | |||
| export const badgeListingColumns: (rootPage: string) => ReadonlyArray<TableColumn<GameBadge>> = (rootPage: string) => [ | |||
There was a problem hiding this comment.
The function signature for badgeListingColumns has changed from a constant array to a function returning an array. Ensure that all usages of badgeListingColumns are updated to call it as a function with the required rootPage argument.
| { | ||
| 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 renderer prop now uses an inline function to pass props to BadgeListingNameRenderer. Verify that BadgeListingNameRenderer can handle the spread operator with the GameBadge type correctly.
| type: 'element', | ||
| }, | ||
| { | ||
| renderer: BadgeActionRenderer, | ||
| renderer: (data: GameBadge) => <BadgeActionRenderer {...data} rootPage={rootPage} />, |
There was a problem hiding this comment.
The renderer prop for BadgeActionRenderer now includes rootPage as a prop. Ensure that BadgeActionRenderer is updated to accept and correctly utilize the rootPage prop.
| @@ -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 imported but not used in a way that optimizes performance. Ensure that the computation inside useMemo is expensive enough to warrant its use.
| url: '#', | ||
| }, | ||
| ]) | ||
| interface Props extends GameBadge { |
There was a problem hiding this comment.
The Props interface extends GameBadge, but it is not clear if all properties of GameBadge are necessary for CreateBadgePage. Consider specifying only the required properties to improve clarity and maintainability.
| @@ -37,16 +34,16 @@ const CreateBadgePage: FC = () => { | |||
| <ContentLayout | |||
There was a problem hiding this comment.
The Breadcrumb component has been removed. Ensure that this is intentional and that the breadcrumb navigation is no longer needed on this page.
| <div className={styles.container}> | ||
| <CreateBadgeForm | ||
| formDef={createBadgeFormDef} | ||
| formDef={formDef} |
There was a problem hiding this comment.
The formDef prop has been changed from createBadgeFormDef to formDef. Verify that formDef is correctly defined and imported in this context.
| onSave={onSave} | ||
| /> | ||
| </div> | ||
| { | ||
| createdBadge && ( | ||
| <BadgeCreatedModal | ||
| rootPage={props.rootPage} |
There was a problem hiding this comment.
The rootPage prop has been added to BadgeCreatedModal. Ensure that props.rootPage is correctly passed and utilized within the BadgeCreatedModal component.
| @@ -3,7 +3,6 @@ import { noop } from 'lodash' | |||
| import { FormDefinition, IconOutline, validatorRequired } from '~/libs/ui' | |||
|
|
|||
| import { ACCEPTED_BADGE_MIME_TYPES, MAX_BADGE_IMAGE_FILE_SIZE } from '../../../config' | |||
| import { rootRoute } from '../../../gamification-admin.routes' | |||
|
|
|||
There was a problem hiding this comment.
The import for rootRoute has been removed. Ensure that this removal is intentional and that rootRoute is not used elsewhere in the file. If it is used, it should be re-imported.
| @@ -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 type annotation for the rootPage parameter is redundant since TypeScript can infer it from the parameter's usage. Consider removing : string.
| @@ -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 in this file to avoid potential runtime errors.
| @@ -90,4 +89,4 @@ export const createBadgeFormDef: FormDefinition = { | |||
| }, | |||
| renderGroupDividers: false, | |||
| }, | |||
| } | |||
| } as FormDefinition) | |||
There was a problem hiding this comment.
The type assertion as FormDefinition is added here. Ensure that this type assertion is necessary and that the createBadgeFormDef variable cannot be inferred as FormDefinition automatically by TypeScript. Unnecessary type assertions can lead to less type safety.
| @@ -34,13 +32,11 @@ export const platformRoutes: Array<PlatformRoute> = [ | |||
| ...devCenterRoutes, | |||
| ...copilotsRoutes, | |||
| ...learnRoutes, | |||
There was a problem hiding this comment.
The removal of ...gamificationAdminRoutes suggests that gamification routes are being consolidated elsewhere. Ensure that these routes are indeed being handled in the new location to prevent any loss of functionality.
| @@ -34,13 +32,11 @@ export const platformRoutes: Array<PlatformRoute> = [ | |||
| ...devCenterRoutes, | |||
| ...copilotsRoutes, | |||
| ...learnRoutes, | |||
| ...gamificationAdminRoutes, | |||
| ...talentSearchRoutes, | |||
| ...profilesRoutes, | |||
| ...walletRoutes, | |||
| ...walletAdminRoutes, | |||
| ...accountsRoutes, | |||
There was a problem hiding this comment.
The removal of ...skillsManagerRoutes indicates that skills manager routes are being consolidated. Verify that these routes are included in the new system-admin app to maintain their availability.
| @@ -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 introduction of a setTimeout with a delay of 100 milliseconds may lead to potential timing issues or race conditions, especially if the component relies on synchronous updates. Consider explaining the necessity of this delay or exploring alternative solutions that do not require setTimeout, such as using a useEffect dependency array to ensure the update occurs at the correct time.
topcoder.atlassian.net/browse/PM-1387
topcoder.atlassian.net/browse/PM-1386