Topcoder Admin App - Misc Update 0518 #1085
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -1,11 +1,11 @@ | |||
| /** | |||
| * Billing accounts page. | |||
| */ | |||
| import { FC } from 'react' | |||
| import { FC, useState } from 'react' | |||
There was a problem hiding this comment.
The useState hook is imported but not used in the current code. Consider removing it if it's not needed.
| import classNames from 'classnames' | ||
|
|
||
| import { PlusIcon } from '@heroicons/react/solid' | ||
| import { LinkButton, LoadingSpinner, PageDivider, PageTitle } from '~/libs/ui' | ||
| import { colWidthType, LinkButton, LoadingSpinner, PageDivider, PageTitle } from '~/libs/ui' |
There was a problem hiding this comment.
The colWidthType import is added but not used in the current code. Consider removing it if it's not needed.
| @@ -22,6 +22,7 @@ interface Props { | |||
| const pageTitle = 'Billing Accounts' | |||
|
|
|||
| export const BillingAccountsPage: FC<Props> = (props: Props) => { | |||
| const [colWidth, setColWidth] = useState<colWidthType>({}) | |||
There was a problem hiding this comment.
The useState hook is initialized with an empty object, but it's unclear what colWidthType is. Ensure that colWidthType is defined and matches the expected shape of the state object.
| @@ -1,10 +1,10 @@ | |||
| /** | |||
| * Billing account clients page. | |||
| */ | |||
| import { FC } from 'react' | |||
| import { FC, useState } from 'react' | |||
There was a problem hiding this comment.
The useState hook is imported but not used in the current code. If it's not needed, consider removing it to keep the imports clean.
| import classNames from 'classnames' | ||
|
|
||
| import { LinkButton, LoadingSpinner, PageDivider, PageTitle } from '~/libs/ui' | ||
| import { colWidthType, LinkButton, LoadingSpinner, PageDivider, PageTitle } from '~/libs/ui' |
There was a problem hiding this comment.
The colWidthType is imported but not used in the current code. If it's not needed, consider removing it to keep the imports clean.
| @@ -1,11 +1,11 @@ | |||
| /** | |||
| * Billing account resources table. | |||
| */ | |||
| import { FC, useMemo } from 'react' | |||
| import { FC, useMemo, useState } from 'react' | |||
There was a problem hiding this comment.
The useState hook is imported but not used in the code. Consider removing it if it's not needed.
| import classNames from 'classnames' | ||
|
|
||
| import { useWindowSize, WindowSize } from '~/libs/shared' | ||
| import { Button, Table, TableColumn } from '~/libs/ui' | ||
| import { Button, colWidthType, Table, TableColumn } from '~/libs/ui' |
There was a problem hiding this comment.
The colWidthType is imported but not used in the code. Consider removing it if it's not needed.
| @@ -100,6 +103,9 @@ export const BillingAccountResourcesTable: FC<Props> = (props: Props) => { | |||
| onToggleSort={setSort} | |||
| forceSort={sort} | |||
| removeDefaultSort | |||
| className={styles.desktopTable} | |||
| colWidth={colWidth} | |||
There was a problem hiding this comment.
The colWidth prop is being added to the component. Ensure that this prop is being used correctly within the component to adjust column widths as expected.
| @@ -100,6 +103,9 @@ export const BillingAccountResourcesTable: FC<Props> = (props: Props) => { | |||
| onToggleSort={setSort} | |||
| forceSort={sort} | |||
| removeDefaultSort | |||
| className={styles.desktopTable} | |||
| colWidth={colWidth} | |||
| setColWidth={setColWidth} | |||
There was a problem hiding this comment.
The setColWidth prop is being added to the component. Verify that this function is being used appropriately to update the column widths dynamically.
| @@ -23,12 +23,15 @@ interface Props { | |||
| setPage: Dispatch<SetStateAction<number>> | |||
| sort: Sort | undefined, | |||
| setSort: Dispatch<SetStateAction<Sort | undefined>> | |||
| colWidth: colWidthType | undefined, | |||
There was a problem hiding this comment.
The colWidth prop is added but not used within the component. Consider utilizing this prop to adjust column widths dynamically if intended.
| @@ -23,12 +23,15 @@ interface Props { | |||
| setPage: Dispatch<SetStateAction<number>> | |||
| sort: Sort | undefined, | |||
| setSort: Dispatch<SetStateAction<Sort | undefined>> | |||
| colWidth: colWidthType | undefined, | |||
| setColWidth: Dispatch<SetStateAction<colWidthType>> | undefined | |||
There was a problem hiding this comment.
The setColWidth prop is added but not used within the component. Ensure this prop is necessary for the component's functionality or remove it if not needed.
| } | ||
|
|
||
| export const BillingAccountsTable: FC<Props> = (props: Props) => { | ||
| const columns = useMemo<TableColumn<BillingAccount>[]>( | ||
| () => [ | ||
| { | ||
| columnId: 'id', |
There was a problem hiding this comment.
The addition of columnId: 'id' seems redundant if the column ID is not used elsewhere in the code. Ensure that this addition is necessary for functionality or remove it if not needed.
| @@ -167,6 +175,9 @@ export const BillingAccountsTable: FC<Props> = (props: Props) => { | |||
| onToggleSort={props.setSort} | |||
| removeDefaultSort | |||
| forceSort={props.sort} | |||
| colWidth={props.colWidth} | |||
There was a problem hiding this comment.
The colWidth prop is being added to the component, but ensure that this prop is being used correctly within the component to adjust column widths as intended.
| @@ -167,6 +175,9 @@ export const BillingAccountsTable: FC<Props> = (props: Props) => { | |||
| onToggleSort={props.setSort} | |||
| removeDefaultSort | |||
| forceSort={props.sort} | |||
| colWidth={props.colWidth} | |||
| setColWidth={props.setColWidth} | |||
There was a problem hiding this comment.
The setColWidth prop is introduced, but verify that it is being utilized appropriately within the component to update column widths dynamically.
| @@ -167,6 +175,9 @@ export const BillingAccountsTable: FC<Props> = (props: Props) => { | |||
| onToggleSort={props.setSort} | |||
| removeDefaultSort | |||
| forceSort={props.sort} | |||
| colWidth={props.colWidth} | |||
| setColWidth={props.setColWidth} | |||
| className={styles.desktopTable} | |||
There was a problem hiding this comment.
The className prop is set to styles.desktopTable. Ensure that the corresponding CSS class is defined and applied correctly to style the table as expected.
| @@ -332,6 +332,8 @@ const ChallengeList: FC<ChallengeListProps> = props => { | |||
| columns={columns} | |||
| data={props.challenges} | |||
| disableSorting | |||
| onToggleSort={_.noop} | |||
There was a problem hiding this comment.
The onToggleSort prop is set to _.noop, which is a no-operation function. If sorting functionality is not needed, consider removing this prop entirely to avoid confusion.
| @@ -23,12 +23,15 @@ interface Props { | |||
| setPage: Dispatch<SetStateAction<number>> | |||
| sort: Sort | undefined | |||
| setSort: Dispatch<SetStateAction<Sort | undefined>> | |||
| colWidth: colWidthType | undefined, | |||
There was a problem hiding this comment.
The colWidth prop is marked as undefined, but it might be better to ensure that it always has a default value or is required, to avoid potential runtime errors when accessing it.
| @@ -23,12 +23,15 @@ interface Props { | |||
| setPage: Dispatch<SetStateAction<number>> | |||
| sort: Sort | undefined | |||
| setSort: Dispatch<SetStateAction<Sort | undefined>> | |||
| colWidth: colWidthType | undefined, | |||
| setColWidth: Dispatch<SetStateAction<colWidthType>> | undefined | |||
There was a problem hiding this comment.
The setColWidth prop is marked as undefined, but it might be better to ensure that it always has a default value or is required, to avoid potential runtime errors when calling it.
| } | ||
|
|
||
| export const ClientsTable: FC<Props> = (props: Props) => { | ||
| const columns = useMemo<TableColumn<ClientInfo>[]>( | ||
| () => [ | ||
| { | ||
| columnId: 'ClientID', |
There was a problem hiding this comment.
The addition of columnId: 'ClientID' should be checked for consistency with other column definitions. Ensure that all columns have a columnId if this is a new requirement.
| @@ -161,6 +169,9 @@ export const ClientsTable: FC<Props> = (props: Props) => { | |||
| onToggleSort={props.setSort} | |||
| removeDefaultSort | |||
| forceSort={props.sort} | |||
| className={styles.desktopTable} | |||
There was a problem hiding this comment.
The className prop is being set to styles.desktopTable. Ensure that styles.desktopTable is defined in your CSS module and that it is being imported correctly. If it's not defined, it may lead to a runtime error.
| @@ -161,6 +169,9 @@ export const ClientsTable: FC<Props> = (props: Props) => { | |||
| onToggleSort={props.setSort} | |||
| removeDefaultSort | |||
| forceSort={props.sort} | |||
| className={styles.desktopTable} | |||
| colWidth={props.colWidth} | |||
There was a problem hiding this comment.
The colWidth prop is being added. Verify that the component receiving this prop is designed to handle it correctly. If this is a new prop, ensure that it is documented and that its purpose is clear.
| @@ -161,6 +169,9 @@ export const ClientsTable: FC<Props> = (props: Props) => { | |||
| onToggleSort={props.setSort} | |||
| removeDefaultSort | |||
| forceSort={props.sort} | |||
| className={styles.desktopTable} | |||
| colWidth={props.colWidth} | |||
| setColWidth={props.setColWidth} | |||
There was a problem hiding this comment.
The setColWidth prop is being added. Ensure that this function is correctly implemented and that it updates the column width as intended. Also, verify that it does not introduce any side effects or performance issues.
| @@ -158,6 +158,7 @@ export const DialogEditUserGroups: FC<Props> = (props: Props) => { | |||
| data={userGroups} | |||
| disableSorting | |||
| onToggleSort={_.noop} | |||
| className={styles.desktopTable} | |||
There was a problem hiding this comment.
Consider verifying that styles.desktopTable is defined and correctly imported. If it is not defined or imported, it could lead to runtime errors or styling issues.
| @@ -154,6 +154,7 @@ export const DialogEditUserRoles: FC<Props> = (props: Props) => { | |||
| data={userRoles} | |||
| disableSorting | |||
| onToggleSort={_.noop} | |||
| className={styles.desktopTable} | |||
There was a problem hiding this comment.
Consider verifying if the styles.desktopTable class is defined and applied correctly to ensure consistent styling across different components.
| @@ -41,3 +41,9 @@ | |||
| padding: 16px 16px 32px; | |||
| text-align: center; | |||
| } | |||
|
|
|||
| .desktopTable { | |||
There was a problem hiding this comment.
Consider adding a class for the table element itself to ensure that the styles are applied correctly to the intended table structure. This can help avoid potential conflicts with other table styles.
|
|
||
| .desktopTable { | ||
| td { | ||
| vertical-align: middle; |
There was a problem hiding this comment.
Consider using vertical-align: middle; on th elements as well, if applicable, to ensure consistent alignment across headers and data cells.
| @@ -1,12 +1,12 @@ | |||
| /** | |||
| * Group members table. | |||
| */ | |||
| import { FC, useCallback, useEffect, useMemo } from 'react' | |||
| import { FC, useCallback, useEffect, useMemo, useState } from 'react' | |||
There was a problem hiding this comment.
The useState hook is imported but not used in the code. If it is not needed, consider removing it to keep the imports clean.
| import _ from 'lodash' | ||
| import classNames from 'classnames' | ||
|
|
||
| import { useWindowSize, WindowSize } from '~/libs/shared' | ||
| import { Button, InputCheckbox, Table, TableColumn } from '~/libs/ui' | ||
| import { Button, colWidthType, InputCheckbox, Table, TableColumn } from '~/libs/ui' |
There was a problem hiding this comment.
The colWidthType is imported but not used in the code. If it is not needed, consider removing it to keep the imports clean.
| @@ -31,10 +31,11 @@ interface Props { | |||
| toggleSelect: (key: number) => void | |||
| forceSelect: (key: number) => void | |||
| forceUnSelect: (key: number) => void | |||
| doRemoveGroupMember: (memberId: number) => void | |||
| doRemoveGroupMember: (memberId: number, memberType: string) => void | |||
There was a problem hiding this comment.
The doRemoveGroupMember function signature has been updated to include a new parameter memberType. Ensure that all calls to this function throughout the codebase are updated to pass the correct memberType argument, and verify that the logic within the function handles this new parameter appropriately.
| } | ||
|
|
||
| export const GroupMembersTable: FC<Props> = (props: Props) => { | ||
| const [colWidth, setColWidth] = useState<colWidthType>({}) |
There was a problem hiding this comment.
The useState hook is used to manage the state of colWidth. Ensure that the colWidthType is correctly defined and imported, and verify that the state management logic for colWidth is implemented correctly throughout the component.
| @@ -150,14 +154,15 @@ export const GroupMembersTable: FC<Props> = (props: Props) => { | |||
| type: 'element', | |||
| }, | |||
| { | |||
| columnId: 'createdAtString', | |||
| label: 'Created at', | |||
| propertyName: 'createdAtString', | |||
| type: 'text', | |||
There was a problem hiding this comment.
The propertyName for the 'Modified By' column seems to have been removed. Ensure that the correct propertyName is set for this column to avoid any potential issues with data rendering.
| @@ -174,19 +179,21 @@ export const GroupMembersTable: FC<Props> = (props: Props) => { | |||
| type: 'element', | |||
| }, | |||
| { | |||
| columnId: 'updatedAtString', | |||
There was a problem hiding this comment.
The addition of columnId: 'updatedAtString' seems redundant as the propertyName already serves a similar purpose. Consider removing columnId or ensuring its necessity.
| label: 'Modified at', | ||
| propertyName: 'updatedAtString', | ||
| type: 'text', | ||
| }, | ||
| { | ||
| columnId: 'action', |
There was a problem hiding this comment.
The columnId: 'action' is added without a label. Ensure that this is intentional and that the absence of a label does not affect the user interface or accessibility.
| label: '', | ||
| renderer: (data: UserGroupMember) => ( | ||
| <Button | ||
| primary | ||
| variant='danger' | ||
| disabled={props.isRemoving[data.memberId]} | ||
| onClick={function onClick() { | ||
| props.doRemoveGroupMember(data.memberId) | ||
| props.doRemoveGroupMember(data.memberId, props.memberType) |
There was a problem hiding this comment.
The function props.doRemoveGroupMember now takes an additional parameter props.memberType. Ensure that this change is reflected in the function definition and that props.memberType is always available and correctly passed. If props.memberType can be undefined or null, consider adding checks or default values to handle such cases.
| @@ -368,6 +375,9 @@ export const GroupMembersTable: FC<Props> = (props: Props) => { | |||
| onToggleSort={setSort} | |||
| forceSort={sort} | |||
| removeDefaultSort | |||
| className={styles.desktopTable} | |||
There was a problem hiding this comment.
The addition of className={styles.desktopTable} suggests that styling is being applied to the table. Ensure that styles.desktopTable is defined and correctly imported to avoid runtime errors.
| @@ -368,6 +375,9 @@ export const GroupMembersTable: FC<Props> = (props: Props) => { | |||
| onToggleSort={setSort} | |||
| forceSort={sort} | |||
| removeDefaultSort | |||
| className={styles.desktopTable} | |||
| colWidth={colWidth} | |||
There was a problem hiding this comment.
The colWidth prop is being added to the component. Verify that colWidth is initialized and used appropriately within the component to prevent potential issues with undefined values.
| @@ -368,6 +375,9 @@ export const GroupMembersTable: FC<Props> = (props: Props) => { | |||
| onToggleSort={setSort} | |||
| forceSort={sort} | |||
| removeDefaultSort | |||
| className={styles.desktopTable} | |||
| colWidth={colWidth} | |||
| setColWidth={setColWidth} | |||
There was a problem hiding this comment.
The setColWidth prop is introduced, which implies a function to update column width. Ensure that setColWidth is defined and correctly handles updates to avoid unexpected behavior.
| @@ -1,11 +1,11 @@ | |||
| /** | |||
| * Groups table. | |||
| */ | |||
| import { FC, useMemo } from 'react' | |||
| import { FC, useMemo, useState } from 'react' | |||
There was a problem hiding this comment.
The useState hook is imported but not used in the current code. Consider removing the import if it's not needed.
| import { Link } from 'react-router-dom' | ||
| import classNames from 'classnames' | ||
|
|
||
| import { Table, TableColumn } from '~/libs/ui' | ||
| import { colWidthType, Table, TableColumn } from '~/libs/ui' |
There was a problem hiding this comment.
The colWidthType import is added but not used in the current code. Consider removing the import if it's not necessary.
| @@ -23,9 +23,11 @@ interface Props { | |||
| } | |||
|
|
|||
| export const GroupsTable: FC<Props> = (props: Props) => { | |||
| const [colWidth, setColWidth] = useState<colWidthType>({}) | |||
There was a problem hiding this comment.
The useState hook is initialized with an empty object for colWidth. Ensure that this is the intended initial state and that it aligns with the expected type colWidthType. If colWidthType is not an object, consider initializing it with a more appropriate default value.
| const columns = useMemo<TableColumn<UserGroup>[]>( | ||
| () => [ | ||
| { | ||
| columnId: 'id', |
There was a problem hiding this comment.
The addition of columnId: 'id' seems to be a new property for the column definition. Verify that this property is supported by the table component being used and that it serves a necessary purpose in the context of the application.
| @@ -47,13 +50,14 @@ export const GroupsTable: FC<Props> = (props: Props) => { | |||
| }, | |||
| { | |||
| className: styles.blockCellWrap, | |||
| columnId: 'description', | |||
There was a problem hiding this comment.
The columnId property is added here for the 'description' column. Ensure that this new property is handled correctly in the rest of the codebase where the table is used, as it might affect sorting or filtering functionalities.
| label: 'Description', | ||
| propertyName: 'description', | ||
| type: 'text', | ||
| }, | ||
| { | ||
| columnId: 'createdByHandle', |
There was a problem hiding this comment.
The columnId property is added here for the 'Created By' column. Verify that this change is consistent with how other columns are defined and ensure that any related logic for column identification is updated accordingly.
| @@ -70,13 +74,14 @@ export const GroupsTable: FC<Props> = (props: Props) => { | |||
| type: 'element', | |||
| }, | |||
| { | |||
| columnId: 'createdAtString', | |||
| label: 'Created at', | |||
| propertyName: 'createdAtString', | |||
There was a problem hiding this comment.
The propertyName for the 'Modified By' column was removed. If this was intentional, ensure that the renderer function handles all necessary logic to display the correct data. If not, consider restoring the propertyName to maintain consistency with other columns.
| @@ -235,6 +241,9 @@ export const GroupsTable: FC<Props> = (props: Props) => { | |||
| onToggleSort={setSort} | |||
| removeDefaultSort | |||
| forceSort={sort} | |||
| className={styles.desktopTable} | |||
There was a problem hiding this comment.
The className prop is being added to the component. Ensure that styles.desktopTable is defined in your styles and is applied correctly to achieve the desired styling.
| @@ -235,6 +241,9 @@ export const GroupsTable: FC<Props> = (props: Props) => { | |||
| onToggleSort={setSort} | |||
| removeDefaultSort | |||
| forceSort={sort} | |||
| className={styles.desktopTable} | |||
| colWidth={colWidth} | |||
There was a problem hiding this comment.
The colWidth prop is being introduced. Verify that this prop is being used correctly within the component to adjust column widths as intended.
| @@ -235,6 +241,9 @@ export const GroupsTable: FC<Props> = (props: Props) => { | |||
| onToggleSort={setSort} | |||
| removeDefaultSort | |||
| forceSort={sort} | |||
| className={styles.desktopTable} | |||
| colWidth={colWidth} | |||
| setColWidth={setColWidth} | |||
There was a problem hiding this comment.
The setColWidth prop is being added. Ensure that this function is defined and correctly updates the column width state when invoked.
| @@ -145,6 +145,7 @@ export const ResourceTable: FC<Props> = (props: Props) => { | |||
| removeDefaultSort | |||
| onToggleSort={props.setSort} | |||
| forceSort={props.sort} | |||
| className={styles.desktopTable} | |||
There was a problem hiding this comment.
Ensure that styles.desktopTable is defined in the styles module. If it is not defined, it may lead to unexpected styling issues.
| @@ -1,9 +1,9 @@ | |||
| import { FC, useMemo } from 'react' | |||
| import { FC, useMemo, useState } from 'react' | |||
There was a problem hiding this comment.
It seems like useState was added to the imports, but there is no usage of useState in the visible code changes. Ensure that useState is actually needed, or remove it if it is not used.
| import { useNavigate } from 'react-router-dom' | ||
|
|
||
| import { EnvironmentConfig } from '~/config' | ||
| import { useWindowSize, WindowSize } from '~/libs/shared' | ||
| import { Button, LinkButton, Table, type TableColumn } from '~/libs/ui' | ||
| import { Button, colWidthType, LinkButton, Table, type TableColumn } from '~/libs/ui' |
There was a problem hiding this comment.
The import of colWidthType was added, but there is no visible usage of colWidthType in the code changes. Verify if colWidthType is necessary, and remove it if it is not used.
| @@ -61,6 +61,7 @@ const ChallengeTitle: FC<{ | |||
| } | |||
|
|
|||
| const ReviewSummaryList: FC<ReviewListProps> = props => { | |||
| const [colWidth, setColWidth] = useState<colWidthType>({}) | |||
There was a problem hiding this comment.
Consider providing a default value for colWidth that matches the expected structure of colWidthType. This ensures that the state is initialized correctly and avoids potential issues with undefined properties.
| @@ -87,7 +90,12 @@ const ReviewSummaryList: FC<ReviewListProps> = props => { | |||
| // propertyName: '', | |||
| // type: 'text', | |||
| // }, | |||
| { label: 'Status', propertyName: 'challengeStatus', type: 'text' }, | |||
| { | |||
| columnId: 'challengeStatus', | |||
There was a problem hiding this comment.
Consider removing the columnId property if it is not being used elsewhere in the code. Adding unused properties can lead to confusion and maintenance overhead.
| @@ -24,9 +24,11 @@ interface Props { | |||
| } | |||
|
|
|||
| export const RolesTable: FC<Props> = (props: Props) => { | |||
| const [colWidth, setColWidth] = useState<colWidthType>({}) | |||
There was a problem hiding this comment.
The useState hook is initialized with an empty object for colWidth. Ensure that this is the intended initial state and that it aligns with how colWidth will be used later in the component.
| const columns = useMemo<TableColumn<UserRole>[]>( | ||
| () => [ | ||
| { | ||
| columnId: 'id', |
There was a problem hiding this comment.
The addition of columnId: 'id' seems to be intended for uniquely identifying the column. Verify that this change is compatible with the rest of the table implementation and that it does not conflict with any existing column identifiers.
| @@ -50,8 +53,8 @@ export const RolesTable: FC<Props> = (props: Props) => { | |||
| }, | |||
| { | |||
| className: styles.blockCellWrap, | |||
| columnId: 'createdByHandle', | |||
There was a problem hiding this comment.
The columnId property is added here, but it seems redundant since propertyName is already used for identifying the column. Consider removing columnId if it is not necessary for functionality.
| @@ -68,14 +71,15 @@ export const RolesTable: FC<Props> = (props: Props) => { | |||
| type: 'element', | |||
| }, | |||
| { | |||
| columnId: 'createdAtString', | |||
| label: 'Created at', | |||
| propertyName: 'createdAtString', | |||
| type: 'text', | |||
| }, | |||
There was a problem hiding this comment.
The propertyName attribute for the 'Modified By' column has been removed. Ensure that the renderer function correctly handles the absence of propertyName and that the data is still being passed correctly to the renderer.
| @@ -229,6 +234,9 @@ export const RolesTable: FC<Props> = (props: Props) => { | |||
| showExpand={false} | |||
| removeDefaultSort | |||
| forceSort={sort} | |||
| className={styles.desktopTable} | |||
There was a problem hiding this comment.
The className prop is added here, but ensure that styles.desktopTable is defined and correctly imported to avoid any runtime errors.
| @@ -229,6 +234,9 @@ export const RolesTable: FC<Props> = (props: Props) => { | |||
| showExpand={false} | |||
| removeDefaultSort | |||
| forceSort={sort} | |||
| className={styles.desktopTable} | |||
| colWidth={colWidth} | |||
There was a problem hiding this comment.
The colWidth prop is introduced, verify that it is being used correctly within the component and that its value is properly initialized and updated as needed.
| @@ -229,6 +234,9 @@ export const RolesTable: FC<Props> = (props: Props) => { | |||
| showExpand={false} | |||
| removeDefaultSort | |||
| forceSort={sort} | |||
| className={styles.desktopTable} | |||
| colWidth={colWidth} | |||
| setColWidth={setColWidth} | |||
There was a problem hiding this comment.
The setColWidth prop is added, check that it is a function and that it is used appropriately to update the column width state. Ensure that it does not cause unnecessary re-renders or side effects.
| @@ -9,6 +9,7 @@ import moment from 'moment' | |||
| import { useWindowSize, WindowSize } from '~/libs/shared' | |||
| import { | |||
| Button, | |||
| colWidthType, | |||
There was a problem hiding this comment.
The import colWidthType should be alphabetically sorted with the other imports. Consider moving it to maintain the order.
| @@ -183,6 +194,7 @@ export const UsersTable: FC<Props> = props => { | |||
| type: 'element', | |||
| }, | |||
| { | |||
| columnId: 'activationCode', | |||
There was a problem hiding this comment.
The addition of the activationCode column seems to be missing any logic or functionality that handles the display or processing of this column's data. Ensure that the necessary data handling and rendering logic is implemented for this new column.
| @@ -212,6 +224,7 @@ export const UsersTable: FC<Props> = props => { | |||
| ...(isMobile | |||
| ? [ | |||
| { | |||
| columnId: 'active', | |||
There was a problem hiding this comment.
Consider checking if the new column 'active' is correctly integrated with the existing data structure and rendering logic. Ensure that the 'active' property exists in the data source and is handled appropriately in the table rendering logic.
| @@ -352,6 +367,9 @@ export const UsersTable: FC<Props> = props => { | |||
| onToggleSort={setSort} | |||
| showExpand | |||
| removeDefaultSort | |||
| className={styles.desktopTable} | |||
There was a problem hiding this comment.
The className prop is being set to styles.desktopTable. Ensure that styles.desktopTable is defined and correctly imported. If it is not defined, this could lead to a runtime error.
| @@ -352,6 +367,9 @@ export const UsersTable: FC<Props> = props => { | |||
| onToggleSort={setSort} | |||
| showExpand | |||
| removeDefaultSort | |||
| className={styles.desktopTable} | |||
| colWidth={colWidth} | |||
There was a problem hiding this comment.
The colWidth prop is being added to the component. Verify that this prop is supported by the component and that it is being used correctly within the component's implementation.
| @@ -352,6 +367,9 @@ export const UsersTable: FC<Props> = props => { | |||
| onToggleSort={setSort} | |||
| showExpand | |||
| removeDefaultSort | |||
| className={styles.desktopTable} | |||
| colWidth={colWidth} | |||
| setColWidth={setColWidth} | |||
There was a problem hiding this comment.
The setColWidth prop is being added to the component. Ensure that this function is defined and correctly handles the logic for setting column widths. Also, verify that the component supports this prop.
| @@ -381,7 +381,7 @@ export interface useManagePermissionGroupMembersProps { | |||
| filterCriteria: FormGroupMembersFilters, | |||
| memberType: string, | |||
| ) => void | |||
| doRemoveGroupMember: (memberId: number) => void | |||
| doRemoveGroupMember: (memberId: number, memberType: string) => void | |||
There was a problem hiding this comment.
The function doRemoveGroupMember now takes an additional parameter memberType. Ensure that all calls to this function throughout the codebase are updated to pass this new parameter to avoid runtime errors.
| @@ -590,16 +590,23 @@ export function useManagePermissionGroupMembers( | |||
| ) | |||
|
|
|||
| const doRemoveGroupMember = useCallback( | |||
| (memberId: number) => { | |||
| (memberId: number, memberType: string) => { | |||
There was a problem hiding this comment.
Consider renaming the memberType parameter to something more descriptive, such as entityType, to better reflect its purpose and improve code readability.
| toastId: 'Remove group member', | ||
| }) | ||
| if (memberType === 'group') { | ||
| toast.success('Group removed successfully', { |
There was a problem hiding this comment.
The toast ID 'Remove group member' is used for both success messages. Consider using distinct toast IDs for 'Group removed successfully' and 'Member removed successfully' to avoid potential conflicts or confusion in toast notifications.
| @@ -145,10 +145,14 @@ export function useManagePermissionGroups( | |||
| _.forEach(result, group => { | |||
| if (group.createdBy) { | |||
| loadUser(group.createdBy) | |||
| } else { | |||
| group.createdByHandle = '' | |||
There was a problem hiding this comment.
Consider initializing group.createdByHandle to null instead of an empty string for better clarity and consistency, especially if null is used elsewhere to indicate the absence of a value.
| } | ||
|
|
||
| if (group.updatedBy) { | ||
| loadUser(group.updatedBy) | ||
| } else { | ||
| group.updatedByHandle = '' |
There was a problem hiding this comment.
Consider initializing group.updatedByHandle to null instead of an empty string for better clarity and consistency, especially if null is used elsewhere to indicate the absence of a value.
| memberId => ({ id: memberId }), | ||
| ) | ||
| const allRoleMembers = (roleInfo.subjects || []).map(subject => ({ | ||
| handle: subject.handle ?? '', |
There was a problem hiding this comment.
Consider using a more descriptive variable name than subject to improve readability, especially since subject is being used to access handle and userId properties.
| ) | ||
| const allRoleMembers = (roleInfo.subjects || []).map(subject => ({ | ||
| handle: subject.handle ?? '', | ||
| id: `${subject.userId ?? ''}`, |
There was a problem hiding this comment.
The template literal ${subject.userId ?? ''} is used to ensure a string type for id. If userId is always a number, consider converting it to a string explicitly using String(subject.userId) for clarity.
| @@ -157,10 +157,14 @@ export function useManagePermissionRoles( | |||
| _.forEach(result, role => { | |||
| if (role.createdBy) { | |||
| loadUser(role.createdBy) | |||
| } else { | |||
| role.createdByHandle = '' | |||
There was a problem hiding this comment.
Consider initializing createdByHandle to a default value earlier in the code to ensure consistency and avoid potential issues if createdBy is not set.
| } | ||
|
|
||
| if (role.modifiedBy) { | ||
| loadUser(role.modifiedBy) | ||
| } else { | ||
| role.modifiedByHandle = '' |
There was a problem hiding this comment.
Consider initializing modifiedByHandle to a default value earlier in the code to ensure consistency and avoid potential issues if modifiedBy is not set.
| isDateObject = checkIsDateObject(value) | ||
| isNumberObject = checkIsNumberObject(value) | ||
| // eslint-disable-next-line no-useless-return | ||
| return |
There was a problem hiding this comment.
The comment // eslint-disable-next-line no-useless-return suggests that the return statement is unnecessary. Consider removing the return statement if it serves no purpose.
| bValue = Number(bValue) | ||
| } | ||
|
|
||
| if (isDateObject || isNumberObject || isNumberString) { |
There was a problem hiding this comment.
The condition if (isDateObject || isNumberObject || isNumberString) is checking multiple flags. Ensure that this logic is intended and that all three types should be handled in the same way. If different handling is required for each type, consider separating the conditions.
| @@ -16,7 +16,11 @@ export interface UserRole { | |||
| modifiedAt: Date | |||
| modifiedAtString?: string | |||
| modifiedByHandle?: string | |||
| subjects?: string[] | |||
| subjects?: { | |||
There was a problem hiding this comment.
The change from string[] to an array of objects with optional fields (email, handle, userId) is significant. Ensure that all parts of the application that interact with subjects are updated to handle this new structure. This includes any serialization/deserialization logic, database schema changes, and any frontend components that display or manipulate this data.
|
|
||
| /** | ||
| * Check if object is date | ||
| * @param date date object |
There was a problem hiding this comment.
Consider using a more specific type than any for the date parameter to improve type safety.
|
|
||
| /** | ||
| * Check if object is number | ||
| * @param numberObject number object |
There was a problem hiding this comment.
Consider using a more specific type than any for the numberObject parameter to improve type safety.
| * @returns true if object is number | ||
| */ | ||
| export function checkIsNumberObject(numberObject: any): boolean { | ||
| return typeof numberObject === 'number' |
There was a problem hiding this comment.
The function checkIsNumberObject checks if the type is 'number', which checks for primitive numbers, not objects. If the intention is to check for Number objects, consider using instanceof Number.
| export function checkIsStringNumeric(str: string): boolean { | ||
| if (typeof str !== 'string') return false // we only process strings! | ||
| return ( | ||
| !Number.isNaN(str as any) |
There was a problem hiding this comment.
Using str as any with Number.isNaN is not ideal. Consider using Number.isNaN(Number(str)) for better type safety and clarity.
| @@ -152,7 +152,7 @@ export const PermissionGroupMembersPage: FC<Props> = (props: Props) => { | |||
| {(groupMembers[memberType] || []) | |||
| .length === 0 ? ( | |||
| <p className={styles.noRecordFound}> | |||
| No members | |||
| No groups | |||
There was a problem hiding this comment.
The message 'No groups' may be misleading if this section is intended to display members. Consider verifying if the context of this message is correct or if it should remain as 'No members'.
| @@ -18,6 +20,8 @@ function getKey(key: string | number): string { | |||
| return `${key}` | |||
| } | |||
|
|
|||
| export type colWidthType = {[key: string]: number} | |||
There was a problem hiding this comment.
Consider renaming colWidthType to ColWidthType to follow TypeScript naming conventions for types, which typically use PascalCase.
| @@ -30,6 +34,9 @@ interface TableProps<T> { | |||
| readonly onRowClick?: (data: T) => void | |||
| readonly onToggleSort?: (sort: Sort) => void | |||
| readonly removeDefaultSort?: boolean | |||
| readonly colWidth?: colWidthType | undefined, | |||
There was a problem hiding this comment.
Consider renaming colWidth to columnWidth for clarity and consistency with common naming conventions.
| @@ -30,6 +34,9 @@ interface TableProps<T> { | |||
| readonly onRowClick?: (data: T) => void | |||
| readonly onToggleSort?: (sort: Sort) => void | |||
| readonly removeDefaultSort?: boolean | |||
| readonly colWidth?: colWidthType | undefined, | |||
| readonly setColWidth?: Dispatch<SetStateAction<colWidthType>> | undefined | |||
There was a problem hiding this comment.
The setColWidth prop suggests that the component may be managing its own state for column widths. Ensure that this aligns with the intended design pattern and doesn't lead to unnecessary complexity or state management issues.
| @@ -30,6 +34,9 @@ interface TableProps<T> { | |||
| readonly onRowClick?: (data: T) => void | |||
| readonly onToggleSort?: (sort: Sort) => void | |||
| readonly removeDefaultSort?: boolean | |||
| readonly colWidth?: colWidthType | undefined, | |||
| readonly setColWidth?: Dispatch<SetStateAction<colWidthType>> | undefined | |||
| readonly className?: string | |||
There was a problem hiding this comment.
The className prop is added, but ensure that it is utilized correctly in the component to apply custom styles. Verify that it doesn't conflict with existing styles or class names.
| @@ -38,6 +45,11 @@ interface DefaultSortDirectionMap { | |||
|
|
|||
| const Table: <T extends { [propertyName: string]: any }>(props: TableProps<T>) => JSX.Element | |||
| = <T extends { [propertyName: string]: any }>(props: TableProps<T>) => { | |||
| const { width: screenWidth }: WindowSize = useWindowSize() | |||
There was a problem hiding this comment.
Consider destructuring useWindowSize() directly in the function parameters if it's only used for screenWidth. This can make the code cleaner and more concise.
| const { width: screenWidth }: WindowSize = useWindowSize() | ||
| const tableRef = useRef<HTMLTableElement>(null) | ||
| const screenWidthBkRef = useRef<number>(0) | ||
| const colWidthInput = props.colWidth |
There was a problem hiding this comment.
Ensure that colWidthInput is being used appropriately in the component. If it's not used, consider removing it to avoid unnecessary variables.
| const tableRef = useRef<HTMLTableElement>(null) | ||
| const screenWidthBkRef = useRef<number>(0) | ||
| const colWidthInput = props.colWidth | ||
| const setColWidth = props.setColWidth |
There was a problem hiding this comment.
Ensure that setColWidth is being used appropriately in the component. If it's not used, consider removing it to avoid unnecessary variables.
| @@ -61,6 +73,52 @@ const Table: <T extends { [propertyName: string]: any }>(props: TableProps<T>) = | |||
| const [sortedData, setSortedData]: [ReadonlyArray<T>, Dispatch<SetStateAction<ReadonlyArray<T>>>] | |||
| = useState<ReadonlyArray<T>>(props.data) | |||
|
|
|||
| useEffect(() => { | |||
| if (_.isEmpty(colWidthInput) && colWidthInput) { | |||
There was a problem hiding this comment.
The condition _.isEmpty(colWidthInput) && colWidthInput seems contradictory. If colWidthInput is empty, it should not be truthy. Consider revising the logic to ensure it behaves as expected.
| @@ -61,6 +73,52 @@ const Table: <T extends { [propertyName: string]: any }>(props: TableProps<T>) = | |||
| const [sortedData, setSortedData]: [ReadonlyArray<T>, Dispatch<SetStateAction<ReadonlyArray<T>>>] | |||
| = useState<ReadonlyArray<T>>(props.data) | |||
|
|
|||
| useEffect(() => { | |||
| if (_.isEmpty(colWidthInput) && colWidthInput) { | |||
| setTimeout(() => { | |||
There was a problem hiding this comment.
Using setTimeout with a fixed delay can lead to unpredictable behavior, especially if the DOM updates are not synchronized with the delay. Consider using a more reliable method to ensure the DOM is ready before accessing it.
| } | ||
| }, 10) | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
Avoid disabling ESLint rules unless absolutely necessary. Consider refactoring the code to comply with the react-hooks/exhaustive-deps rule, which ensures that all dependencies are correctly specified.
| } | ||
|
|
||
| screenWidthBkRef.current = screenWidth | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
Avoid disabling ESLint rules unless absolutely necessary. Consider refactoring the code to comply with the react-hooks/exhaustive-deps rule, which ensures that all dependencies are correctly specified.
| return ( | ||
| <th | ||
| className={styles.th} | ||
| className={classNames(styles.th, columnId)} |
There was a problem hiding this comment.
The classNames function is used to combine classes, but columnId is not a class name. If columnId is intended to be a class, ensure it is defined in the CSS module. Otherwise, consider whether it should be included here.
| key={getKey(index)} | ||
| > | ||
| <div | ||
| className={ | ||
| classNames(styles['header-container'], styles[col.type], colorClass, sortableClass) | ||
| } | ||
| style={colWidth ? { width: `${colWidth}px` } : {}} |
There was a problem hiding this comment.
The inline style for setting the column width is conditionally applied. Ensure that colWidth is a valid number and consider handling cases where it might not be, to avoid potential rendering issues.
| /> | ||
| )) | ||
|
|
||
| return ( | ||
| /* TODO: sticky header */ | ||
| <div className={styles['table-wrap']}> | ||
| <table className={styles.table}> | ||
| <div className={classNames(styles['table-wrap'], props.className)}> |
There was a problem hiding this comment.
Consider checking if props.className is defined before passing it to classNames to avoid potential issues if it's undefined or null.
| <div className={styles['table-wrap']}> | ||
| <table className={styles.table}> | ||
| <div className={classNames(styles['table-wrap'], props.className)}> | ||
| <table ref={tableRef} className={styles.table}> |
There was a problem hiding this comment.
Ensure that tableRef is properly initialized and used. If it's intended to be a React ref, make sure it's created using useRef and that it's being used correctly elsewhere in the component.
| @@ -92,6 +93,7 @@ const TableCell: <T extends { [propertyName: string]: any }>( | |||
| 'TableCell_blockCell', | |||
| )} | |||
| onClick={onClick} | |||
| style={props.style} | |||
There was a problem hiding this comment.
Consider validating the props.style to ensure it doesn't contain any unexpected values that could affect the layout or styling of the table cell.
| @@ -21,6 +21,7 @@ interface Props<T> { | |||
| readonly columns: ReadonlyArray<TableColumn<T>> | |||
| index: number | |||
| readonly showExpand?: boolean | |||
| colWidth?: {[key: string]: number} | |||
There was a problem hiding this comment.
Consider making colWidth a Readonly type to ensure that the column widths are not accidentally modified after being set. This would align with the columns property which is also marked as ReadonlyArray.
| /> | ||
| )) | ||
| const cells: Array<JSX.Element> = displayColumns.map((col, colIndex) => { | ||
| const columnId = `column-id-${col.columnId}-` |
There was a problem hiding this comment.
The variable columnId is constructed using a template string with col.columnId. Ensure that col.columnId is defined and has a valid value to avoid potential issues with undefined or unexpected values.
| )) | ||
| const cells: Array<JSX.Element> = displayColumns.map((col, colIndex) => { | ||
| const columnId = `column-id-${col.columnId}-` | ||
| const colWidth = props.colWidth?.[columnId] |
There was a problem hiding this comment.
The expression props.colWidth?.[columnId] uses optional chaining. Verify that props.colWidth is always defined when columnId is accessed to prevent potential runtime errors.
| } | ||
| : undefined | ||
| } | ||
| style={colWidth ? { width: `${colWidth}px` } : {}} |
There was a problem hiding this comment.
The style prop is conditionally set based on colWidth. Consider verifying that colWidth is a valid number before using it in the style object to ensure proper rendering of the width.
| col.className, | ||
| 'TableCell_blockExpandValue', | ||
| )} | ||
| style={colWidth ? { width: `${colWidth}px` } : {}} |
There was a problem hiding this comment.
Consider checking if colWidth is a valid number before using it to set the width style. This can prevent potential issues if colWidth is undefined or not a number.
| @@ -34,7 +36,7 @@ const TableSort: FC<TableSortProps> = (props: TableSortProps) => { | |||
|
|
|||
| return ( | |||
| <Button | |||
| className={classNames(props.iconClass, 'TableSort')} | |||
| className={classNames(props.iconClass, 'TableSort', styles.btnSort)} | |||
There was a problem hiding this comment.
Ensure that styles.btnSort is defined and imported correctly. If styles is not imported or btnSort is not defined in the styles, this could lead to runtime errors.
| } | ||
|
|
||
| export function useWindowSize(): WindowSize { | ||
| const [windowSize, setWindowSize]: [WindowSize, Dispatch<SetStateAction<WindowSize>>] = useState({ |
There was a problem hiding this comment.
Consider using useLayoutEffect instead of useEffect to ensure the window size is updated before the browser paints, which can help avoid visual inconsistencies during the initial render.
| }) | ||
|
|
||
| const handleResize: () => void = useCallback(() => { | ||
| // Set window width/height to state |
There was a problem hiding this comment.
The comment here is redundant as the code is self-explanatory. Consider removing it to keep the code clean.
| }, []) | ||
|
|
||
| useEffect(() => { | ||
| // Add event listener |
There was a problem hiding this comment.
The comment here is redundant as the code is self-explanatory. Consider removing it to keep the code clean.
| // Add event listener | ||
| window.addEventListener('resize', handleResize) | ||
|
|
||
| // Call handler right away so state gets updated with initial window size |
There was a problem hiding this comment.
The comment here is redundant as the code is self-explanatory. Consider removing it to keep the code clean.
| // Call handler right away so state gets updated with initial window size | ||
| handleResize() | ||
|
|
||
| // Remove event listener on cleanup |
There was a problem hiding this comment.
The comment here is redundant as the code is self-explanatory. Consider removing it to keep the code clean.
Challenge https://www.topcoder.com/challenges/07eb3683-b62a-4063-82d6-dd66527d7bca?tab=details