PROD - gamification / skills management consolidation. SSO users option #1123
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
Consolidate gamification, skills manager, add SSO users option in system-admin app
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -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.
Consider adding error handling for the useLayout() hook in case it returns an unexpected value or fails to provide the Layout component.
| @@ -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 checked to ensure these paths are correct and that the modules exist in the specified location. If these imports are not used in the file, consider removing them to keep the code clean.
| rootRoute, | ||
| userManagementRouteId, | ||
| } from './config/routes.config' | ||
| import { platformSkillRouteId } from './platform/routes.config' |
There was a problem hiding this comment.
Consider organizing the imports alphabetically for better readability and consistency.
| { | ||
| element: <SkillManagementLandingPage />, | ||
| id: 'skills-landing-page', | ||
| route: '', |
There was a problem hiding this comment.
Consider specifying 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 rootPage prop is correctly constructed and that all variables used in the template literal are defined and imported as needed.
| }, | ||
| { | ||
| element: <CreateBadgePage />, | ||
| route: `${gamificationAdminRouteId}${createBadgePath}`, |
There was a problem hiding this comment.
Verify that createBadgePath is defined and correctly imported to avoid runtime errors.
| }, | ||
| { | ||
| element: <BadgeDetailPage />, | ||
| route: `${gamificationAdminRouteId}${baseDetailPath}/:id`, |
There was a problem hiding this comment.
Ensure that baseDetailPath is defined and correctly imported. Also, confirm that the dynamic segment :id is handled properly in the application logic.
| @@ -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 the styles.blockForm class is defined and used correctly in the stylesheet. Ensure it does not conflict with existing styles or cause unintended layout changes.
| @@ -0,0 +1,71 @@ | |||
| .modal { | |||
| width: 800px !important; | |||
There was a problem hiding this comment.
Avoid using !important unless absolutely necessary, as it can make the CSS harder to maintain and override.
| align-items: flex-start; | ||
|
|
||
| th:first-child { | ||
| padding-left: 16px !important; |
There was a problem hiding this comment.
Using !important should be avoided if possible. Consider refactoring the CSS to achieve the desired effect without it.
| } | ||
|
|
||
| .tableCell { | ||
| white-space: break-spaces !important; |
There was a problem hiding this comment.
The use of !important can lead to specificity issues in CSS. Try to refactor the styles to avoid using it.
|
|
||
| .tableCell { | ||
| white-space: break-spaces !important; | ||
| text-align: left !important; |
There was a problem hiding this comment.
Consider removing !important to maintain cleaner and more manageable CSS.
| width: 100%; | ||
| } | ||
|
|
||
| .btnActions { |
There was a problem hiding this comment.
The .btnActions class appears to have the same styles as .actionButtons. Consider consolidating these classes if they serve the same purpose.
| height: 100px; | ||
|
|
||
| .spinner { | ||
| background: none; |
There was a problem hiding this comment.
The .spinner class is defined twice with the same background: none; style. Consider consolidating these styles to avoid redundancy.
| left: 0; | ||
|
|
||
| .spinner { | ||
| background: none; |
There was a problem hiding this comment.
The .spinner class is defined twice with the same background: none; style. Consider consolidating these styles to avoid redundancy.
| props.setOpen(false) | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [isLoading]) |
There was a problem hiding this comment.
The useCallback dependency array should include props.setOpen to ensure the function is correctly memoized with the latest reference.
| type: 'action', | ||
| }, | ||
| ], | ||
| [isAdding, isRemoving, doRemoveSSOUserLogin], |
There was a problem hiding this comment.
The useMemo dependency array should include setShowEditForm to ensure the memoized value is updated when setShowEditForm changes.
| ] | ||
| }), | ||
| [columns], | ||
| ) |
There was a problem hiding this comment.
The useMemo dependency array should include styles to ensure the memoized value is updated when styles changes.
| const cancelEditForm = useCallback(() => { | ||
| setShowEditForm(undefined) | ||
| setShowAddForm(false) | ||
| }, [setShowAddForm, setShowEditForm]) |
There was a problem hiding this comment.
The useCallback dependency array should include setShowAddForm and setShowEditForm to ensure the function is correctly memoized with the latest references.
| columns={columns} | ||
| data={ssoUserLogins} | ||
| disableSorting | ||
| onToggleSort={_.noop} |
There was a problem hiding this comment.
The onToggleSort prop is set to _.noop, which might be unnecessary if sorting is disabled. Consider removing it if it's not needed.
| userId: props.editingData.userId, | ||
| }) | ||
| } | ||
| }, [props.editingData]) |
There was a problem hiding this comment.
Consider adding reset to the dependency array to avoid potential stale closures or unintended behavior when the reset function changes.
| placeholder='Select' | ||
| options={providerOptions} | ||
| value={controlProps.field.value} | ||
| onChange={controlProps.field.onChange} |
There was a problem hiding this comment.
The onChange handler for InputSelectReact should also handle null or undefined values to prevent potential runtime errors.
| @@ -47,6 +50,7 @@ interface Props { | |||
|
|
|||
| export const UsersTable: FC<Props> = props => { | |||
| const [colWidth, setColWidth] = useState<colWidthType>({}) | |||
| const [ssoLoginProviders, setSsoLoginProviders] = useState<SSOLoginProvider[]>([]) | |||
There was a problem hiding this comment.
Consider initializing ssoLoginProviders with a meaningful default value if applicable, or add a check before using it to ensure it doesn't lead to unexpected behavior when empty.
| @@ -359,6 +370,16 @@ export const UsersTable: FC<Props> = props => { | |||
| [isTablet, isMobile], | |||
| ) | |||
|
|
|||
| useOnComponentDidMount(() => { | |||
There was a problem hiding this comment.
Consider adding error handling logic within the fetchSSOLoginProviders function itself to encapsulate error management, rather than handling it externally in the component.
| setSsoLoginProviders(result) | ||
| }) | ||
| .catch(e => { | ||
| handleError(e) |
There was a problem hiding this comment.
Ensure that the handleError function provides meaningful feedback to the user or logs the error appropriately for debugging purposes.
| {showDialogEditUserSSOLogin && ( | ||
| <DialogEditUserSSOLogin | ||
| open | ||
| setOpen={function setOpen() { |
There was a problem hiding this comment.
Consider using a more descriptive function name for setOpen to clearly indicate its purpose, such as closeSSOLoginDialog.
|
|
||
| @include ltelg { | ||
| padding: 0; | ||
| } |
There was a problem hiding this comment.
The class name .contantentLayoutInner seems to be a typo. It has been corrected to .contentLayoutInner, which is good. Ensure that all references to this class in the codebase are updated accordingly.
| ) | ||
| 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 being used, but it's not clear from the code why this comment is necessary. Consider removing the comment if it's not needed or providing more context on why the ESLint rule is being disabled.
| <>{props.children}</> | ||
| ) | ||
| export const NullLayout: FC<PropsWithChildren> = props => <>{props.children}</> | ||
|
|
There was a problem hiding this comment.
The NullLayout component is defined on a single line, which can reduce readability. Consider formatting it across multiple lines for better clarity and consistency with other component definitions.
| ) | ||
|
|
||
| export function useLayout(): { Layout: FC<LayoutProps> } { | ||
| const routerContextData: RouterContextData = useContext(routerContext) |
There was a problem hiding this comment.
The useLayout function uses useContext to access routerContext, but there is no error handling if routerContext is not available. Consider adding a check or a fallback to handle cases where routerContext might be undefined.
| ? `/${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 potentially lead to issues if the pathname contains encoded characters. Consider using a more robust method to handle path comparisons.
| @@ -3,11 +3,14 @@ import _ from 'lodash' | |||
| import { TabsNavItem } from '~/libs/ui' | |||
| import { | |||
| billingAccountRouteId, | |||
| gamificationAdminRouteId, | |||
There was a problem hiding this comment.
Consider verifying if gamificationAdminRouteId is correctly defined and used in the application, as it is newly added.
| manageChallengeRouteId, | ||
| manageReviewRouteId, | ||
| permissionManagementRouteId, | ||
| platformRouteId, |
There was a problem hiding this comment.
Ensure that platformRouteId is correctly integrated and utilized within the application, as it is a new addition.
| userManagementRouteId, | ||
| } from '~/apps/admin/src/config/routes.config' | ||
| import { platformSkillRouteId } from '~/apps/admin/src/platform/routes.config' |
There was a problem hiding this comment.
Check if platformSkillRouteId is properly defined and used, as it is newly imported from a different module.
| { | ||
| children: [ | ||
| { | ||
|
|
There was a problem hiding this comment.
Remove the extra newline here to maintain consistent formatting with other entries.
| { | ||
| id: `${platformRouteId}/${gamificationAdminRouteId}`, | ||
| title: 'Badges', | ||
|
|
There was a problem hiding this comment.
Remove the extra newline here to maintain consistent formatting with other entries.
| 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 variable for updating to avoid confusion and potential bugs.
| ...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 for SSOUserLogin. Ensure that provider is indeed unique or consider using a more reliable identifier for matching.
|
|
||
| case SSOUserLoginsActionType.REMOVE_SSO_USER_LOGIN_DONE: { | ||
| const ssoUserLogins = _.filter( | ||
| previousState.ssoUserLogins, |
There was a problem hiding this comment.
Using _.filter from lodash is fine, but since this is a simple filtering operation, consider using the native JavaScript filter method for better readability and performance.
| @@ -1,15 +1,18 @@ | |||
| import _ from 'lodash' | |||
|
|
|||
| import { EnvironmentConfig } from '~/config' | |||
| import { xhrGetAsync, xhrPatchAsync } from '~/libs/core' | |||
| import { xhrDeleteAsync, xhrGetAsync, xhrPatchAsync, xhrPostAsync, xhrPutAsync } from '~/libs/core' | |||
There was a problem hiding this comment.
Consider removing the unused import xhrPutAsync if it is not needed in this file.
|
|
||
| import { | ||
| adjustUserInfoResponse, | ||
| adjustUserStatusHistoryResponse, | ||
| ApiV3Response, | ||
| SSOLoginProvider, |
There was a problem hiding this comment.
The SSOLoginProvider import is added but not used in the current code. Ensure it is necessary or remove it if not used.
|
|
||
| import { | ||
| adjustUserInfoResponse, | ||
| adjustUserStatusHistoryResponse, | ||
| ApiV3Response, | ||
| SSOLoginProvider, | ||
| SSOUserLogin, |
There was a problem hiding this comment.
The SSOUserLogin import is added but not used in the current code. Ensure it is necessary or remove it if not used.
| UserInfo, | ||
| UserStatusHistory, | ||
| } from '../models' | ||
| import { FormAddSSOLoginData } from '../models/FormAddSSOLoginData.model' |
There was a problem hiding this comment.
The FormAddSSOLoginData import is added but not used in the current code. Ensure it is necessary or remove it if not used.
| } | ||
|
|
||
| /** | ||
| * Fetch list of sso login provider. |
There was a problem hiding this comment.
The comment for the fetchSSOLoginProviders function states that it returns 'sso user logins', but it should be 'sso login providers'.
| * @param userId user id. | ||
| * @param userLogin user login info. | ||
| * @returns resolves to sso user login | ||
| */ |
There was a problem hiding this comment.
The comment for the createSSOUserLogin function states that it returns 'sso user login', but it should specify that it returns 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.
The comment for the updateSSOUserLogin function states that it returns 'sso user login', but it should specify that it returns 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.
The comment for the deleteSSOUserLogin function states that it returns 'sso user login', but it should specify that it returns the deleted 'sso user login'.
| */ | ||
| export const formAddSSOLoginSchema: Yup.ObjectSchema<FormAddSSOLoginData> | ||
| = Yup.object({ | ||
| email: Yup.string() |
There was a problem hiding this comment.
Consider adding a .max() constraint to limit the length of the email string to a reasonable number of characters, as email addresses have a practical length limit.
| .trim() | ||
| .email('Invalid email address.') | ||
| .required('Email address is required.'), | ||
| name: Yup.string() |
There was a problem hiding this comment.
Consider adding a .max() constraint to limit the length of the name string to prevent excessively long inputs.
| name: Yup.string() | ||
| .trim() | ||
| .required('Name is required.'), | ||
| provider: Yup.string() |
There was a problem hiding this comment.
Consider adding a .max() constraint to limit the length of the provider string to prevent excessively long inputs.
| provider: Yup.string() | ||
| .trim() | ||
| .required('Provider is required.'), | ||
| userId: Yup.string() |
There was a problem hiding this comment.
Consider adding a .max() constraint to limit the length of the userId string to prevent excessively long inputs.
| () => 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 dependency array is empty, which might lead to stale closures if getRouteElement or adminRoutes change. Consider adding getRouteElement and adminRoutes as dependencies to ensure the memoized value is updated correctly.
|
|
||
| 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. Ensure that the new path is correct and that the context module has been moved to the lib directory.
| @@ -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.
Suggestion:
Consider using a semantic HTML element like <main> instead of <div> for the main content area to improve accessibility and provide better context for the content 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 replaced with a div. Ensure that this change is intentional and that the div provides the same styling and layout functionality as ContentLayout. If ContentLayout has specific styles or logic, consider whether those need to be replicated or replaced.
| ?.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 an empty dependency array [] in useMemo means that the memoized value will only be recalculated once, when the component mounts. If getRouteElement or any other variables used inside the useMemo function can change, they should be included in the dependency array to ensure the memoized value updates 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 badgeDetailPath function signature seems to have changed to accept an additional parameter props.rootPage. Ensure that this change is reflected in the function definition and all its usages throughout the codebase to prevent potential runtime errors.
| 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 function badgeDetailPath now requires an additional rootPage parameter. Ensure that all calls to this function in the codebase are updated to pass the correct rootPage value.
| route: rootRoute, | ||
| }, | ||
| ] | ||
| export const createBadgeRoute: (rootPage: string) => string = ( |
There was a problem hiding this comment.
The createBadgeRoute is now a function that takes rootPage as a parameter. Ensure that all usages of createBadgeRoute are updated to call it as a function with the appropriate rootPage argument.
| @@ -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 used in the rest of the file to avoid runtime errors.
| 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 an argument, 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 memoize initColumns. Verify that badgeListingColumns is a pure function to ensure that useMemo works as expected and does not cause unnecessary re-renders.
| } | ||
| const BadgeDetailPage: FC<Props> = (props: Props) => { | ||
| const initColumns = useMemo(() => badgeListingColumns(props.rootPage), [props.rootPage]) | ||
| const navigate = useNavigate() |
There was a problem hiding this comment.
The useNavigate hook is used here. Ensure that the react-router-dom version being used supports this hook, as it was introduced in version 6.
| @@ -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. Please ensure that initColumns is properly defined or imported in the file to avoid potential 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 not being properly tracked in the useEffect hook. Consider explicitly listing all dependencies that the effect relies on to ensure it behaves as expected.
| @@ -201,6 +199,7 @@ const BadgeDetailPage: FC = () => { | |||
| setShowActivatedModal(true) | |||
| onBadgeUpdated() | |||
| }) | |||
| // eslint-disable-next-line no-alert | |||
| .catch(e => alert(`onActivateBadge error: ${e.message}`)) | |||
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 notification system or logging the error for debugging purposes.
| @@ -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 notification system or logging the error for debugging purposes.
| 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, which is more robust than using a relative path.
| @@ -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 is imported but not used in the code. If it's not needed, consider removing it to keep the imports clean and avoid confusion.
| 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 TypeScript can infer the type from the function signature. Consider removing the explicit type annotation for 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 and does not have side effects, as this could lead to 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 props is not defined or imported in the current scope. Ensure that props is correctly defined or passed to the component.
| 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 diff. Ensure that it is utilized in the component or remove it if it is unnecessary.
| @@ -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 badgeDetailPath function signature seems to have changed, now requiring props.rootPage and props.id as arguments. Ensure that these props are correctly passed and available in the component. If props.id is intended to replace badge.id, verify that it holds the correct value.
| @@ -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 change from a constant array to a function returning an array is correct, but ensure that all usages of badgeListingColumns are updated accordingly to pass the rootPage parameter.
| { | ||
| 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 function now uses JSX syntax to render BadgeListingNameRenderer. Ensure that BadgeListingNameRenderer is a valid React component and can accept the spread data props.
| type: 'element', | ||
| }, | ||
| { | ||
| renderer: BadgeActionRenderer, | ||
| renderer: (data: GameBadge) => <BadgeActionRenderer {...data} rootPage={rootPage} />, |
There was a problem hiding this comment.
The renderer function now uses JSX syntax to render BadgeActionRenderer with additional rootPage prop. Ensure that BadgeActionRenderer is a valid React component and can accept both the spread data props and 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 justify its use.
| url: '#', | ||
| }, | ||
| ]) | ||
| interface Props extends GameBadge { |
There was a problem hiding this comment.
The Props interface extends GameBadge, but it's not clear if all properties of GameBadge are necessary for CreateBadgePage. Consider explicitly defining only the required properties to improve code readability and maintainability.
| <div className={styles.container}> | ||
| <CreateBadgeForm | ||
| formDef={createBadgeFormDef} | ||
| formDef={formDef} | ||
| onSave={onSave} |
There was a problem hiding this comment.
The variable formDef is used here, but it's not clear from the diff if it has been defined or imported correctly. Please ensure that formDef is properly defined or imported in the component.
| onSave={onSave} | ||
| /> | ||
| </div> | ||
| { | ||
| createdBadge && ( | ||
| <BadgeCreatedModal | ||
| rootPage={props.rootPage} |
There was a problem hiding this comment.
The prop rootPage is being added to BadgeCreatedModal. Ensure that BadgeCreatedModal component is updated to accept this new prop and handle it appropriately.
| @@ -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, but it is not clear if it was unused or if its removal was intentional. Please verify if this import is no longer needed or if it should be replaced with another import.
| @@ -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 createBadgeFormDef function is repeated. Consider removing the redundant type annotation for the parameter rootPage: string as it is already inferred from the function signature.
| @@ -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 change from rootRoute || '/' to rootPage removes the fallback to the root path ('/'). Ensure that rootPage is always defined and valid, or consider maintaining a fallback to prevent potential navigation issues.
| @@ -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 used here. Ensure that this is necessary and that the createBadgeFormDef object conforms to the FormDefinition type. If the object already matches the type, consider removing the type assertion to avoid unnecessary complexity.
| @@ -1,9 +1,7 @@ | |||
| export enum AppSubdomain { | |||
| skillsManager = 'manage', | |||
| accounts = 'account-settings', | |||
There was a problem hiding this comment.
The skillsManager subdomain has been removed. Ensure that this is intentional and that there are no dependencies or references to this subdomain elsewhere in the codebase.
| accounts = 'account-settings', | ||
| devCenter = 'devcenter', | ||
| earn = 'earn', | ||
| gamificationAdmin = 'gamification-admin', | ||
| profiles = 'profiles', | ||
| tcAcademy = 'academy', |
There was a problem hiding this comment.
The gamificationAdmin subdomain has been removed. Verify that this removal is intentional and that there are no remaining dependencies or references to this subdomain in other parts of the application.
| @@ -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 to delay the updateOffset function call may introduce potential issues such as race conditions or unexpected behavior if the component's state changes during the delay. Consider providing a rationale for this change or exploring alternative solutions that do not rely on arbitrary time delays.
https://topcoder.atlassian.net/browse/PM-1386
https://topcoder.atlassian.net/browse/PM-1387