Topcoder Admin App - Add Terms Management Final fix #1133
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
|
|
||
| .dialogLoadingSpinnerContainer { | ||
| position: absolute; |
There was a problem hiding this comment.
Consider using a more descriptive class name for .dialogLoadingSpinnerContainer to clearly indicate its purpose or context within the dialog.
| left: 0; | ||
|
|
||
| .spinner { | ||
| background: none; |
There was a problem hiding this comment.
The .spinner class currently only sets background: none;. If there are additional styles needed for the spinner, consider adding them here for clarity and completeness.
| const onSubmit = useCallback( | ||
| (data: FormAddTermUser) => { | ||
| props.doAddTermUser( | ||
| data.handle?.value ?? 0, |
There was a problem hiding this comment.
Consider checking if data.handle is defined before accessing value and label to avoid potential runtime errors.
| onBlur={controlProps.field.onBlur} | ||
| classNameWrapper={styles.inputField} | ||
| disabled={props.isAdding} | ||
| dirty |
There was a problem hiding this comment.
The dirty prop in FieldHandleSelect is set to true, but it is not clear if this prop is necessary or used within the component. Verify if this prop is needed and remove it if it is not used.
| placeholder?: string | ||
| readonly value?: SelectOption | ||
| readonly value?: SelectOption | null |
There was a problem hiding this comment.
Consider updating the type definition for value to SelectOption | null in any other related interfaces or components that might be affected by this change to ensure consistency across the codebase.
| @@ -16,8 +16,9 @@ import styles from './FieldSingleSelectAsync.module.scss' | |||
| interface Props { | |||
| label?: string | |||
| className?: string | |||
| classNameWrapper?: string | |||
There was a problem hiding this comment.
Consider adding a description for the new classNameWrapper prop in the component's documentation or prop types to clarify its purpose and usage.
| align-items: center; | ||
| justify-content: center; | ||
| height: 64px; | ||
| left: $sp-8; |
There was a problem hiding this comment.
Consider using a more descriptive variable name instead of $sp-8 to improve readability and maintainability.
| justify-content: center; | ||
| height: 64px; | ||
| left: $sp-8; | ||
| bottom: $sp-8; |
There was a problem hiding this comment.
Consider using a more descriptive variable name instead of $sp-8 to improve readability and maintainability.
| } | ||
|
|
||
| @include ltelg { | ||
| left: $sp-4; |
There was a problem hiding this comment.
Consider using a more descriptive variable name instead of $sp-4 to improve readability and maintainability.
|
|
||
| @include ltelg { | ||
| left: $sp-4; | ||
| bottom: $sp-4; |
There was a problem hiding this comment.
Consider using a more descriptive variable name instead of $sp-4 to improve readability and maintainability.
| */ | ||
| const onSubmit = useCallback( | ||
| (data: FormAddTerm) => { | ||
| const requestBody = _.pickBy(data, _.identity) |
There was a problem hiding this comment.
Consider adding error handling for the doAddTerm and doUpdateTerm functions to manage potential failures during the term addition or update process.
|
|
||
| const agreeabilityTypeId = watch('agreeabilityTypeId') | ||
| useEffect(() => { | ||
| // check to enable/disable 'Docusign Template ID' and 'URL' fields |
There was a problem hiding this comment.
The useEffect dependency array should include getValues and setValue to ensure that the effect runs correctly when these functions change.
| }, [agreeabilityTypeId]) | ||
|
|
||
| useEffect(() => { | ||
| if (termInfo) { |
There was a problem hiding this comment.
The useEffect dependency array should include reset to ensure that the effect runs correctly when this function changes.
| <Button | ||
| primary | ||
| onClick={function onClick() { | ||
| setShowEditor(prev => !prev) |
There was a problem hiding this comment.
Consider using a more descriptive function name for the onClick handler to improve readability and maintainability.
|
|
||
| .fields { | ||
| display: flex; | ||
| gap: 15px; |
There was a problem hiding this comment.
Consider using a variable for the gap value instead of a hardcoded 15px to maintain consistency and ease of updates across the stylesheet.
|
|
||
| .blockBottom { | ||
| display: flex; | ||
| gap: 10px; |
There was a problem hiding this comment.
Consider using a variable for the gap value instead of a hardcoded 10px to maintain consistency and ease of updates across the stylesheet.
| .blockBottom { | ||
| display: flex; | ||
| gap: 10px; | ||
| margin-top: 3px; |
There was a problem hiding this comment.
Consider using a variable for the margin-top value instead of a hardcoded 3px to maintain consistency and ease of updates across the stylesheet.
| label='Title' | ||
| placeholder='Enter' | ||
| tabIndex={0} | ||
| onChange={_.noop} |
There was a problem hiding this comment.
The onChange prop is set to _.noop, which means it does nothing. If this is intentional, consider removing it to avoid confusion.
| secondary | ||
| onClick={function onClick() { | ||
| reset(defaultValues) | ||
| setTimeout(() => { |
There was a problem hiding this comment.
The setTimeout function is used without a delay parameter. If the intention is to execute onSubmit asynchronously, consider specifying a delay or using a different approach to ensure clarity.
| color: $blue-110; | ||
|
|
||
| &:hover { | ||
| color: $blue-110; |
There was a problem hiding this comment.
The &:hover selector is setting the same color as the default link color. Consider using a different color for hover to provide visual feedback to users.
| } | ||
|
|
||
| .tableCell { | ||
| white-space: break-spaces !important; |
There was a problem hiding this comment.
Using !important can make the CSS harder to maintain. Consider refactoring the CSS to avoid using !important if possible.
|
|
||
| .tableCell { | ||
| white-space: break-spaces !important; | ||
| text-align: left !important; |
There was a problem hiding this comment.
Using !important can make the CSS harder to maintain. Consider refactoring the CSS to avoid using !important if possible.
| totalPages: number | ||
| page: number | ||
| setPage: Dispatch<SetStateAction<number>> | ||
| colWidth: colWidthType | undefined |
There was a problem hiding this comment.
Consider providing a default value for colWidth to avoid potential undefined behavior.
| page: number | ||
| setPage: Dispatch<SetStateAction<number>> | ||
| colWidth: colWidthType | undefined | ||
| setColWidth: Dispatch<SetStateAction<colWidthType>> | undefined |
There was a problem hiding this comment.
Consider providing a default value for setColWidth to avoid potential undefined behavior.
| type: 'element', | ||
| }, | ||
| ], | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
The dependency array for useMemo is empty, which means the memoized value will never update. Ensure this is intentional, or consider adding dependencies if the columns need to update based on props or state changes.
| ...column, | ||
| className: '', | ||
| label: `${column.label as string} label`, | ||
| mobileType: 'label', |
There was a problem hiding this comment.
The label for mobile columns is being suffixed with 'label'. Ensure this is the intended behavior, as it might lead to redundancy in the UI.
| .fields { | ||
| display: grid; | ||
| grid-template-columns: repeat(4, minmax(0, 1fr)); | ||
| gap: 15px 30px; |
There was a problem hiding this comment.
Consider using CSS variables or a pre-defined spacing variable for the gap values to maintain consistency across the application.
| display: flex; | ||
| justify-content: flex-end; | ||
| align-items: flex-start; | ||
| gap: 15px; |
There was a problem hiding this comment.
Consider using CSS variables or a pre-defined spacing variable for the gap values to maintain consistency across the application.
| label='User Id' | ||
| placeholder='Enter' | ||
| tabIndex={0} | ||
| onChange={_.noop} |
There was a problem hiding this comment.
The onChange handler is set to _.noop, which means it does nothing. If this is intentional, consider removing the onChange prop entirely to avoid confusion.
| label='Handle' | ||
| placeholder='Enter' | ||
| tabIndex={0} | ||
| onChange={_.noop} |
There was a problem hiding this comment.
Similar to the previous comment, the onChange handler is set to _.noop. If no action is needed on change, consider omitting this prop.
| onClick={function onClick() { | ||
| reset(defaultValues) | ||
| setTimeout(() => { | ||
| onSubmit(defaultValues) |
There was a problem hiding this comment.
The setTimeout function is used without a delay parameter, which defaults to 0. This might be unnecessary if the intention is to execute onSubmit immediately after reset. Consider removing setTimeout if no delay is needed.
| } | ||
|
|
||
| .tableCell { | ||
| white-space: break-spaces !important; |
There was a problem hiding this comment.
Using !important can make it difficult to override styles in the future. Consider if this is necessary or if the specificity of the selector can be increased instead.
|
|
||
| .tableCell { | ||
| white-space: break-spaces !important; | ||
| text-align: left !important; |
There was a problem hiding this comment.
Using !important can make it difficult to override styles in the future. Consider if this is necessary or if the specificity of the selector can be increased instead.
| props.forceSelect(item.userId) | ||
| }) | ||
| } | ||
| }, [isSelectAll, props.datas]) |
There was a problem hiding this comment.
Consider adding props.forceUnSelect and props.forceSelect to the dependency array of toggleSelectAll to ensure that the latest versions of these functions are used.
| ] | ||
| }), | ||
| [columns], | ||
| ) |
There was a problem hiding this comment.
Consider adding props.isRemovingBool and props.isRemoving to the dependency array of columnsMobile to ensure that the latest values are used.
| // extend body ultra small and override it | ||
| font-size: 14px; | ||
| line-height: 19px; | ||
| line-height: $error-line-height; |
There was a problem hiding this comment.
The line-height property is defined twice on this line. Consider removing the redundant line-height: 19px; to avoid confusion and ensure consistency.
| const elementRef = useRef<HTMLTextAreaElement>(null) | ||
| const easyMDE = useRef<any>(null) | ||
|
|
||
| /** |
There was a problem hiding this comment.
Consider specifying the type for easyMDE instead of using any to improve type safety and maintainability.
| }, | ||
| [], | ||
| ) | ||
|
|
There was a problem hiding this comment.
The getState function uses a lot of string manipulation and regular expressions. Consider adding unit tests to ensure its correctness and robustness.
| * Handle toggle block | ||
| */ | ||
| const toggleBlock = useCallback( | ||
| (editor: any, type: string, startChars: any, endCharsInput?: any) => { |
There was a problem hiding this comment.
The toggleBlock function is complex and handles multiple cases. Consider breaking it down into smaller functions to improve readability and maintainability.
| /** | ||
| * Show hint after '@' | ||
| */ | ||
| const completeAfter = useCallback((cm: CodeMirror.Editor) => { |
There was a problem hiding this comment.
The completeAfter function directly manipulates the editor content. Ensure that this behavior is well-tested to prevent unexpected side effects.
| }, []) | ||
|
|
||
| useOnComponentDidMount(() => { | ||
| easyMDE.current = new EasyMDE({ |
There was a problem hiding this comment.
The useOnComponentDidMount hook initializes the EasyMDE editor. Ensure that the dependencies are correctly managed to prevent potential memory leaks or unexpected behavior.
| uploadImage: false, | ||
| }) | ||
|
|
||
| easyMDE.current.codemirror.on('change', (cm: CodeMirror.Editor) => { |
There was a problem hiding this comment.
Consider adding error handling for the change and blur events to gracefully handle any potential issues during these events.
| @import '@libs/ui/styles/includes'; | ||
|
|
||
| .form-input-textarea { | ||
| @include font-roboto; |
There was a problem hiding this comment.
The font-roboto mixin should be checked to ensure it is correctly imported and available in the context of this file. If it is not defined in the included files, this will cause a compilation error.
| border: none; | ||
| outline: none; | ||
| resize: vertical; | ||
| margin-left: calc(-1 * $border); |
There was a problem hiding this comment.
The use of calc(-1 * $border) for margin-left should be verified to ensure that $border is defined and that this calculation results in the intended layout. If $border is not defined or is not a numeric value, this could lead to unexpected behavior.
|
|
||
| useEffect(() => { | ||
| if (!initValue) { | ||
| setInitValue(props.value as string) |
There was a problem hiding this comment.
The setInitValue function is called only when initValue is falsy, which means it will not update if props.value changes after the initial render. Consider whether this behavior is intended or if you need to update initValue whenever props.value changes.
| {...props} | ||
| dirty={!!props.dirty} | ||
| disabled={!!props.disabled} | ||
| label={props.label || props.name} |
There was a problem hiding this comment.
The label prop defaults to props.name if props.label is not provided. Ensure that props.name is always a suitable fallback for the label, as it might not always be user-friendly.
| @@ -16,14 +16,19 @@ | |||
| display: flex; | |||
| gap: 15px; | |||
| justify-content: flex-end; | |||
| flex-wrap: wrap; | |||
There was a problem hiding this comment.
Consider if flex-wrap: wrap; is necessary here. If the container's content is not expected to exceed the container's width, this property might be redundant.
|
|
||
| @include ltemd { | ||
| grid-template-columns: 1fr; | ||
| display: flex; |
There was a problem hiding this comment.
The display: flex; property is being set within the @include ltemd media query. Ensure that this change is intended, as it overrides the display: grid; property set earlier. This could affect the layout significantly.
| @@ -14,7 +14,7 @@ | |||
| padding: $sp-4; | |||
| } | |||
|
|
|||
| &.isPlatformPage { | |||
| &.isPlatformGamificationAdminPage { | |||
There was a problem hiding this comment.
The class name change from isPlatformPage to isPlatformGamificationAdminPage should be verified across the codebase to ensure that all references to this class are updated accordingly. This will prevent any potential styling issues due to mismatched class names.
| @@ -1,11 +1,10 @@ | |||
| import { FC, PropsWithChildren, useContext } from 'react' | |||
| import cn from 'classnames' | |||
|
|
|||
| import { platformRouteId } from '~/apps/admin/src/config/routes.config' | |||
| import { gamificationAdminRouteId, platformRouteId, rootRoute } from '~/apps/admin/src/config/routes.config' | |||
There was a problem hiding this comment.
The import statement for gamificationAdminRouteId and rootRoute has been added, but it's not clear if these are used in the current file. Please ensure that all imported modules are utilized to avoid unnecessary imports.
| import { ContentLayout } from '~/libs/ui' | ||
| import { routerContext, RouterContextData } from '~/libs/core' | ||
| import { platformSkillRouteId } from '~/apps/admin/src/platform/routes.config' | ||
| import { AppSubdomain, EnvironmentConfig } from '~/config' | ||
|
|
There was a problem hiding this comment.
The import statement for AppSubdomain and EnvironmentConfig has been removed. Verify if these are no longer needed in the file, as removing them might affect the functionality if they were used elsewhere in the code.
| @@ -39,8 +38,8 @@ export const Layout: FC<LayoutProps> = props => ( | |||
| </ContentLayout> | |||
| ) | |||
|
|
|||
| export const PlatformLayout: FC<LayoutProps> = props => ( | |||
| <Layout classes={{ mainClass: styles.isPlatformPage }}> | |||
| export const PlatformGamificationAdminLayout: FC<LayoutProps> = props => ( | |||
There was a problem hiding this comment.
The component name PlatformGamificationAdminLayout is quite long and may reduce readability. Consider using a shorter, more concise name if possible.
| export const PlatformLayout: FC<LayoutProps> = props => ( | ||
| <Layout classes={{ mainClass: styles.isPlatformPage }}> | ||
| export const PlatformGamificationAdminLayout: FC<LayoutProps> = props => ( | ||
| <Layout classes={{ mainClass: styles.isPlatformGamificationAdminPage }}> |
There was a problem hiding this comment.
Ensure that styles.isPlatformGamificationAdminPage is defined in your styles module. If it is not, this could lead to runtime errors.
| const skillManagementRouteId = EnvironmentConfig.SUBDOMAIN === AppSubdomain.admin | ||
| ? `/${platformRouteId}/${platformSkillRouteId}` | ||
| : `/${AppSubdomain.admin}/${platformRouteId}/${platformSkillRouteId}` | ||
| const platformBasePath = `${rootRoute}/${platformRouteId}/${gamificationAdminRouteId}` |
There was a problem hiding this comment.
The variable platformBasePath is declared but never used in the code. Consider removing it if it's not needed, or ensure it is used appropriately if it is required for functionality.
| .startsWith(platformBaseRouteId.toLowerCase())) { | ||
| return { Layout: PlatformLayout } | ||
| .startsWith(platformBasePath.toLowerCase())) { | ||
| return { Layout: PlatformGamificationAdminLayout } |
There was a problem hiding this comment.
The change from PlatformLayout to PlatformGamificationAdminLayout should be verified to ensure that it aligns with the intended functionality of the terms management feature. Make sure that PlatformGamificationAdminLayout provides the necessary layout and components required for the admin interface.
| @@ -20,6 +21,7 @@ export const PageWrapper: FC<PropsWithChildren<Props>> = props => ( | |||
| <PageTitle>{props.pageTitle}</PageTitle> | |||
| <PageHeader> | |||
| <h3 className={styles.textTitle}>{props.pageTitle}</h3> | |||
| {props.pageSubTitle} | |||
There was a problem hiding this comment.
Consider wrapping props.pageSubTitle in a conditional check to ensure it is not undefined or null before rendering. This will prevent potential rendering issues if pageSubTitle is not provided.
| */ | ||
| const doUpdateTerm = useCallback( | ||
| (data: Partial<FormAddTerm>, callBack: () => void) => { | ||
| setIsAdding(true) |
There was a problem hiding this comment.
Suggestion
Consider using a more descriptive variable name instead of reusing isAdding for the update operation. This can help improve code readability and avoid confusion, as isAdding is also used in the doAddTerm function.
Example
You could introduce a new state variable like isUpdating for the update operation:
const [isUpdating, setIsUpdating] = useState(false);And then use it in the doUpdateTerm function:
setIsUpdating(true);
...
setIsUpdating(false);Reasoning
This change will make it clear that the state is specifically related to the update operation, rather than the add operation.
| type: TermsActionType.FETCH_TERMS_INIT, | ||
| }) | ||
| let filter = `page=${pagRequest}&perPage=${TABLE_PAGINATION_ITEM_PER_PAGE}` | ||
| if (filterCriteria?.searchKey) { |
There was a problem hiding this comment.
Consider using const instead of let for the filter variable since it is not reassigned.
| let filter = `page=${pagRequest}&perPage=${TABLE_PAGINATION_ITEM_PER_PAGE}` | ||
| if (filterCriteria?.searchKey) { | ||
| filter += `&title=${filterCriteria?.searchKey}` | ||
| } |
There was a problem hiding this comment.
The optional chaining operator ?. is used twice on filterCriteria?.searchKey. The second usage is redundant because if filterCriteria is undefined, the first check will already prevent accessing searchKey.
| type: TermsActionType.FETCH_TERMS_DONE, | ||
| }) | ||
| success() | ||
| window.scrollTo({ left: 0, top: 0 }) |
There was a problem hiding this comment.
The window.scrollTo function is called with an object. Ensure that this behavior is intended and supported across all target browsers.
| } from '../models' | ||
| import { handleError } from '../utils' | ||
| import { | ||
| addUserTerm, |
There was a problem hiding this comment.
Consider renaming addUserTerm to addTermUser for consistency with other function names like removeTermUser.
| } | ||
| | { | ||
| type: typeof TermsActionType.FETCH_TERMS_USERS_DONE | ||
| payload: { |
There was a problem hiding this comment.
The payload type here could be more specific. Consider defining a type for the payload to improve type safety and readability.
| | typeof TermsActionType.REMOVE_TERMS_USERS_DONE | ||
| | typeof TermsActionType.REMOVE_TERMS_USERS_INIT | ||
| | typeof TermsActionType.REMOVE_TERMS_USERS_FAILED | ||
| payload: number |
There was a problem hiding this comment.
The payload type here is a number, which is inconsistent with the other actions that use objects. Consider using a consistent structure for all action payloads.
| const requestSuccess = (data: number[], totalPages: number): void => { | ||
| dispatch({ | ||
| payload: { | ||
| data: data.map(item => ({ |
There was a problem hiding this comment.
Consider using a more descriptive variable name than item to improve code readability.
| && filterCriteria?.userId.toString() | ||
| .trim() | ||
| ) { | ||
| filter += `&userId=${filterCriteria.userId}` |
There was a problem hiding this comment.
The toString().trim() method chain is repeated multiple times. Consider extracting this logic into a utility function to improve code readability and maintainability.
|
|
||
| if (filterCriteria?.handle && filterCriteria?.handle.trim()) { | ||
| if ( | ||
| filterCriteria?.userId |
There was a problem hiding this comment.
The condition filterCriteria?.userId && filterCriteria?.userId.toString().trim() is repeated. Consider extracting this logic into a utility function or variable to avoid repetition.
| ) | ||
|
|
||
| useEffect(() => { | ||
| _.forEach(state.datas, termUser => { |
There was a problem hiding this comment.
The dependency array for this useEffect is missing some dependencies like loadUser, setPage, and reloadData. Ensure all dependencies are included to avoid potential bugs.
| @@ -99,6 +100,7 @@ export function useTableFilterBackend<T>( | |||
|
|
|||
| return { | |||
| page, | |||
| reloadData: doSearchDatas, | |||
There was a problem hiding this comment.
The function doSearchDatas seems to be used here, but its definition or import is not visible in this diff. Ensure that doSearchDatas is correctly defined or imported in the file to avoid runtime errors.
| @@ -3,5 +3,13 @@ | |||
| */ | |||
| export interface UserTerm { | |||
| id: string | |||
| legacyId: number | |||
There was a problem hiding this comment.
Consider using a consistent type for IDs. If id is a string, ensure legacyId and typeId are also string unless there's a specific reason for them to be number.
| title: string | ||
| url: string | ||
| agreeabilityTypeId: string |
There was a problem hiding this comment.
The agreeabilityTypeId is a string, but typeId is a number. Ensure consistency in ID types unless there's a specific reason for the difference.
| > => { | ||
| const result = await xhrGetPaginatedAsync<{ | ||
| result: number[] | ||
| }>(`${EnvironmentConfig.API.V5}/terms/${termId}/users?${filter ?? ''}`) |
There was a problem hiding this comment.
The filter parameter is appended directly to the URL without any encoding. Consider using encodeURIComponent to ensure that the filter string is safely included in the URL.
| @@ -20,14 +27,14 @@ import { FormAddSSOLoginData } from '../models/FormAddSSOLoginData.model' | |||
| */ | |||
| export const getMemberSuggestionsByHandle = async ( | |||
| handle: string, | |||
| ): Promise<Array<{ handle: string; userId: number }>> => { | |||
| ): Promise<Array<MemberInfo>> => { | |||
There was a problem hiding this comment.
The type MemberInfo should be defined or imported if not already done. Ensure that MemberInfo includes the properties handle and userId as expected by the function.
| @@ -38,13 +45,13 @@ export const getMemberSuggestionsByHandle = async ( | |||
| */ | |||
| export const getMembersByHandle = async ( | |||
| handles: string[], | |||
| ): Promise<Array<{ handle: string }>> => { | |||
| ): Promise<Array<MemberInfo>> => { | |||
There was a problem hiding this comment.
The return type of the function has been changed from Array<{ handle: string }> to Array<MemberInfo>. Ensure that the MemberInfo type is correctly defined and includes all necessary fields such as handle and any other fields expected from the API response.
| */ | ||
| export const getProfile = async (handle: string): Promise<MemberInfo> => { | ||
| if (!handle) { | ||
| return Promise.reject(new Error('Handle must be specified.')) |
There was a problem hiding this comment.
The check for an empty handle should throw an error instead of returning a rejected promise. Consider using throw new Error('Handle must be specified.') for better readability and consistency.
| const result = await xhrGetAsync<ApiV3Response<MemberInfo>>( | ||
| `${EnvironmentConfig.API.V3}/members/${handle}`, | ||
| ) | ||
| return result.result.content |
There was a problem hiding this comment.
Consider adding error handling for the xhrGetAsync call to manage potential network or API errors gracefully.
| * @param units units | ||
| * @returns file size | ||
| */ | ||
| export function humanFileSize(inputBytes: number, units: string[]): string { |
There was a problem hiding this comment.
Consider adding input validation for inputBytes and units to ensure they are of expected types and values. For example, inputBytes should be a non-negative number, and units should be a non-empty array.
| */ | ||
| export function humanFileSize(inputBytes: number, units: string[]): string { | ||
| let bytes = inputBytes | ||
| if (Math.abs(bytes) < 1024) { |
There was a problem hiding this comment.
The condition Math.abs(bytes) < 1024 assumes that units has at least one element. Consider adding a check to ensure units is not empty before accessing units[0].
| u += 1 | ||
| } while (Math.abs(bytes) >= 1024 && u < units.length) | ||
|
|
||
| return `${bytes.toFixed(1)}${units[u]}` |
There was a problem hiding this comment.
The toFixed(1) method is used here, which will round the number to one decimal place. Consider whether this level of precision is appropriate for all use cases, or if it should be configurable.
| FormUsersFilters, | ||
| } from '../models' | ||
| import { FormEditUserStatus } from '../models/FormEditUserStatus.model' | ||
| import { FormAddRoleMembers } from '../models/FormAddRoleMembers.type' | ||
| import { FormAddSSOLoginData } from '../models/FormAddSSOLoginData.model' | ||
| import { FormAddTermUser } from '../models/FormAddTermUser.model' | ||
|
|
||
| const docusignTypeId |
There was a problem hiding this comment.
The variable docusignTypeId is declared but not initialized properly. Consider using const docusignTypeId = EnvironmentConfig.ADMIN.AGREE_FOR_DOCUSIGN_TEMPLATE; to ensure proper initialization.
| */ | ||
| export const formAddTermUserSchema: Yup.ObjectSchema<FormAddTermUser> | ||
| = Yup.object({ | ||
| handle: Yup.object() |
There was a problem hiding this comment.
The handle field is defined as an object, but the error message 'Handle is required.' might be misleading if the object itself is present but one of its properties is missing. Consider specifying which part of the handle is required in the error message.
| label: Yup.string() | ||
| .required('Label is required.'), | ||
| value: Yup.number() | ||
| .typeError('Invalid number.') |
There was a problem hiding this comment.
The error message 'Invalid number.' for the value field might not be clear to users. Consider providing more context, such as 'Value must be a valid number.'
| docusignTemplateId: Yup.string() | ||
| .trim() | ||
| .when('agreeabilityTypeId', (agreeabilityTypeId, schema) => { | ||
| if (agreeabilityTypeId[0] === docusignTypeId) { |
There was a problem hiding this comment.
Consider checking if agreeabilityTypeId is an array before accessing its first element with [0]. This will prevent potential runtime errors if agreeabilityTypeId is not an array.
| .required('Type is required.'), | ||
| url: Yup.string() | ||
| .trim() | ||
| .url('Invalid url.') |
There was a problem hiding this comment.
The url validation should be required if the URL is a mandatory field. Consider changing .optional() to .required('URL is required.') if applicable.
| className?: string | ||
| } | ||
|
|
||
| export const TermsAddPage: FC<Props> = (props: Props) => { |
There was a problem hiding this comment.
Consider using destructuring for props to directly extract className. This can make the code cleaner and more readable. For example: export const TermsAddPage: FC<Props> = ({ className }) => {
| } | ||
|
|
||
| .removeSelectionButtonContainer { | ||
| padding: 20px 0 30px $sp-8; |
There was a problem hiding this comment.
The padding value 20px 0 30px $sp-8 mixes fixed pixel values with a variable $sp-8. Consider using consistent units for padding values to maintain uniformity and scalability.
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| bottom: 50px; |
There was a problem hiding this comment.
The bottom: 50px; value is a fixed pixel measurement. Consider using a variable or relative unit to ensure responsiveness and adaptability across different screen sizes.
| } | ||
|
|
||
| export const TermsUsersPage: FC<Props> = (props: Props) => { | ||
| const [showDialogAddUser, setShowDialogAddUser] = useState<boolean>() |
There was a problem hiding this comment.
Consider initializing showDialogAddUser with a default value like false instead of undefined to avoid potential issues with undefined state.
| export const TermsUsersPage: FC<Props> = (props: Props) => { | ||
| const [showDialogAddUser, setShowDialogAddUser] = useState<boolean>() | ||
| useAutoScrollTopWhenInit() | ||
| const { id = '' }: { id?: string } = useParams<{ |
There was a problem hiding this comment.
The destructuring of useParams could be simplified by directly using const { id = '' } = useParams(); without specifying the type again, as TypeScript can infer the type from the hook.
| icon={PlusIcon} | ||
| iconToLeft | ||
| label='Add User' | ||
| onClick={function onClick() { |
There was a problem hiding this comment.
The onClick function can be simplified by using an arrow function: onClick={() => setShowDialogAddUser(true)}.
| variant='danger' | ||
| disabled={!hasSelected || isRemovingBool} | ||
| size='lg' | ||
| onClick={function onClick() { |
There was a problem hiding this comment.
The onClick function can be simplified by using an arrow function: onClick={() => doRemoveTermUsers(selectedDatasArray, unselectAll)}.
| {showDialogAddUser && termInfo && ( | ||
| <DialogAddTermUser | ||
| open | ||
| setOpen={function setOpen() { |
There was a problem hiding this comment.
The setOpen function can be simplified by using an arrow function: setOpen={() => { if (!isAdding) setShowDialogAddUser(false); }}.
| onBlur={() => setStateHasFocus(false)} | ||
| onBlur={() => { | ||
| setStateHasFocus(false) | ||
| props.onBlur?.() |
There was a problem hiding this comment.
Consider adding a check to ensure props.onBlur is a function before calling it. This can prevent potential runtime errors if onBlur is not a function. For example, you could use typeof props.onBlur === 'function' && props.onBlur().
| @@ -156,6 +157,7 @@ const InputSelectReact: FC<InputSelectReactProps> = props => { | |||
| backspaceRemovesValue | |||
| isDisabled={props.disabled} | |||
| filterOption={props.filterOption} | |||
| isLoading={props.isLoading} | |||
There was a problem hiding this comment.
Consider providing a default value for props.isLoading to avoid potential issues if the prop is not passed. This can be done by setting a default value in the component's defaultProps or using a default parameter in the function signature.
| @@ -10,7 +10,6 @@ | |||
| outline: none; | |||
| resize: vertical; | |||
| margin-left: calc(-1 * $border); | |||
| overflow: hidden; | |||
| padding: $border; | |||
There was a problem hiding this comment.
Removing overflow: hidden; might cause the textarea to display scrollbars if the content exceeds the visible area. Ensure that this change aligns with the intended design and user experience for the textarea component.
| @@ -5170,6 +5184,11 @@ | |||
| resolved "https://registry.yarnpkg.com/@types/marked/-/marked-4.0.7.tgz#400a76809fd08c2bbd9e25f3be06ea38c8e0a1d3" | |||
| integrity sha512-eEAhnz21CwvKVW+YvRvcTuFKNU9CV1qH+opcgVK3pIMI6YZzDm6gc8o2vHjldFk6MGKt5pueSB7IOpvpx5Qekw== | |||
|
|
|||
| "@types/marked@^4.0.7": | |||
There was a problem hiding this comment.
It seems like a new entry for @types/marked@^4.0.7 has been added. Ensure that this addition is intentional and necessary for the project, as it might introduce changes in type definitions that could affect the codebase.
| gap: 10px; | ||
| } | ||
|
|
||
| .fieldAreaContainer { |
There was a problem hiding this comment.
The new class .fieldAreaContainer is added but not used in this diff. Ensure that it is utilized in the component or remove it if unnecessary.
|
|
||
| import styles from './BundledEditor.module.scss' | ||
|
|
||
| export const BundledEditor: FC<any> = (props: any) => ( |
There was a problem hiding this comment.
Using any as the type for props is not recommended because it defeats the purpose of TypeScript's type checking. Consider defining a specific type or interface for the props that BundledEditor expects.
| @@ -0,0 +1,78 @@ | |||
| import { FC, FocusEvent, useEffect, useRef, useState } from 'react' | |||
There was a problem hiding this comment.
The useRef hook is imported but not used in the current code. If it's not needed, consider removing it to keep the imports clean.
|
|
||
| import { FormInputAutocompleteOption, InputWrapper } from '~/libs/ui' | ||
|
|
||
| import { BundledEditor } from './BundledEditor' |
There was a problem hiding this comment.
The import path for the editor component has changed from ./Editor to ./BundledEditor. Ensure that BundledEditor provides the same functionality or intended improvements over the previous Editor component.
| const FieldHtmlEditor: FC<FieldHtmlEditorProps> = ( | ||
| props: FieldHtmlEditorProps, | ||
| ) => { | ||
| const editorRef = useRef<any>(null) |
There was a problem hiding this comment.
Using any for the editorRef type can lead to potential type safety issues. Consider specifying a more precise type for the ref, such as React.RefObject<EditorType> where EditorType is the expected type of the editor instance.
| hideInlineErrors={props.hideInlineErrors} | ||
| > | ||
| <BundledEditor | ||
| onInit={function onInit(_evt: any, editor: any) { |
There was a problem hiding this comment.
Consider specifying the types for _evt and editor instead of using any to improve type safety.
| onInit={function onInit(_evt: any, editor: any) { | ||
| (editorRef.current = editor) | ||
| }} | ||
| onChange={function onChange() { |
There was a problem hiding this comment.
The onChange function does not use the _evt parameter. Consider removing it if it's not needed.
| + '}', | ||
| height: 400, | ||
| menubar: false, | ||
| plugins: ['table', 'link', 'textcolor', 'contextmenu'], |
There was a problem hiding this comment.
The source_view option is not a valid TinyMCE configuration option. Consider removing or correcting it.
| import 'tinymce/skins/ui/oxide/skin' // Editor styles | ||
| import 'tinymce/skins/content/default/content' // Content styles, including inline UI like fake cursors | ||
| import 'tinymce/skins/ui/oxide/content' | ||
| import 'tinymce/plugins/table/plugin.min.js' |
There was a problem hiding this comment.
The table plugin import has been moved but not commented out. Ensure this change is intentional and that the plugin is still required.
| import 'tinymce/skins/content/default/content' // Content styles, including inline UI like fake cursors | ||
| import 'tinymce/skins/ui/oxide/content' | ||
| import 'tinymce/plugins/table/plugin.min.js' | ||
| import 'tinymce/plugins/link/plugin.min.js' |
There was a problem hiding this comment.
The link plugin import has been moved but not commented out. Verify if this change is necessary and that the plugin is still needed.
| import 'tinymce/skins/ui/oxide/content' | ||
| import 'tinymce/plugins/table/plugin.min.js' | ||
| import 'tinymce/plugins/link/plugin.min.js' | ||
| // import 'tinymce/plugins/advlist/plugin.min.js' // importing the plugin js. |
There was a problem hiding this comment.
Consider removing the commented-out imports if they are no longer needed to keep the code clean and maintainable.
https://www.topcoder.com/challenges/1acf18a6-af38-4583-9c37-d2fc18d23fa1