PM-3452 Customer Portal App skeleton #1410
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -0,0 +1,5 @@ | |||
| export const PRIVILEGED_ROLES = [ | |||
| 'administrator', | |||
There was a problem hiding this comment.
[correctness]
Consider normalizing role names to a consistent case (e.g., all lowercase) to avoid potential issues with case sensitivity when checking roles elsewhere in the application.
There was a problem hiding this comment.
This is just how roles are configured in our system
| import { AppSubdomain, EnvironmentConfig } from '~/config' | ||
|
|
||
| export const rootRoute: string | ||
| = EnvironmentConfig.SUBDOMAIN === AppSubdomain.customer |
There was a problem hiding this comment.
[correctness]
The use of EnvironmentConfig.SUBDOMAIN assumes that SUBDOMAIN is always defined and valid. Consider adding validation or default values to handle potential undefined or incorrect configurations.
| children: [ | ||
| { | ||
| authRequired: true, | ||
| element: <Rewrite to={talentSearchRouteId} />, |
There was a problem hiding this comment.
[correctness]
Consider verifying that talentSearchRouteId is a valid route identifier at runtime to prevent potential navigation errors.
| } from './config/routes.config' | ||
| import { customerPortalTalentSearchRoutes } from './pages/talent-search/talent-search.routes' | ||
|
|
||
| const CustomerPortalApp: LazyLoadedComponent = lazyLoad(() => import('./CustomerPortalApp')) |
There was a problem hiding this comment.
[performance]
Ensure that the lazy-loaded component CustomerPortalApp is properly handled for loading states and potential errors during the loading process.
src/apps/customer-portal/src/lib/components/BreadCrumb/BreadCrumb.module.scss
Show resolved
Hide resolved
src/apps/customer-portal/src/lib/components/BreadCrumb/BreadCrumb.module.scss
Show resolved
Hide resolved
src/apps/customer-portal/src/lib/components/BreadCrumb/BreadCrumb.module.scss
Show resolved
Hide resolved
| <button | ||
| type='button' | ||
| onClick={function onClick() { | ||
| navigate(item.path as number, { |
There was a problem hiding this comment.
[❗❗ correctness]
The navigate function is called with item.path cast to a number. Ensure that item.path is always a number when item.fallback is present, as incorrect types could lead to runtime errors.
| {item.label} | ||
| </button> | ||
| ) : ( | ||
| <Link to={item.path as string}> |
There was a problem hiding this comment.
[❗❗ correctness]
Casting item.path to a string for the Link component assumes that item.path is always a valid string when item.fallback is not present. Ensure that this assumption holds true to prevent potential navigation issues.
| margin: $sp-6 auto !important; | ||
| } | ||
|
|
||
| .contantentLayoutInner { |
There was a problem hiding this comment.
[❗❗ correctness]
There is a typo in the class name .contantentLayoutInner. It should likely be .contentLayoutInner. This could lead to issues with styling not being applied correctly.
| } | ||
|
|
||
| .contentLayoutOuter { | ||
| margin: $sp-6 auto !important; |
There was a problem hiding this comment.
[maintainability]
Avoid using !important unless absolutely necessary, as it can make styles difficult to override and maintain. Consider refactoring to achieve the desired styling without !important.
| <> | ||
| <NavTabs /> | ||
| <ContentLayout | ||
| innerClass={styles.contantentLayoutInner} |
There was a problem hiding this comment.
[❗❗ correctness]
There is a typo in the class name contantentLayoutInner. It should likely be contentLayoutInner to match the intended naming convention.
| @@ -0,0 +1,2 @@ | |||
| export * from './Layout' | |||
There was a problem hiding this comment.
[maintainability]
Using both export * and named exports from the same module can lead to potential conflicts or unexpected behavior if there are overlapping exports. Consider using only named exports for clarity and to avoid such issues.
| @include ltemd { | ||
| cursor: pointer; | ||
| &::after { | ||
| background: url('../../assets/chevron-down.svg') |
There was a problem hiding this comment.
[❗❗ correctness]
Ensure that the path to chevron-down.svg is correct and that the asset is available at the specified location. Missing assets can lead to broken UI elements.
| } | ||
| } | ||
| .tab { | ||
| background-color: var(--Appeal); |
There was a problem hiding this comment.
[💡 maintainability]
Consider using a more descriptive variable name instead of var(--Appeal) for the background color. This will improve the maintainability and readability of the code by making the purpose of the color more explicit.
| margin-left: 0; | ||
| width: 100%; | ||
| &.active { | ||
| background-color: var(--Actived); |
There was a problem hiding this comment.
[❗❗ correctness]
Ensure that var(--Actived) and var(--invertButtonColor) are defined in the CSS variables. Undefined variables can lead to unexpected styling issues.
|
|
||
| if (tabUrl) { | ||
| setIsOpen(false) | ||
| window.open(tabUrl, '_blank', 'noopener,noreferrer') |
There was a problem hiding this comment.
[security]
Using window.open with '_blank' can expose the application to security risks such as tabnabbing. Ensure that the noopener and noreferrer attributes are always included, which you have done correctly here. Consider using a utility function to handle external links consistently across the application.
| [navigate], | ||
| ) | ||
|
|
||
| useClickOutside(triggerRef.current, () => setIsOpen(false)) |
There was a problem hiding this comment.
[❗❗ correctness]
The useClickOutside hook is used with triggerRef.current, which might be null initially. Ensure that the hook can handle a null reference to avoid potential runtime errors.
| @@ -0,0 +1 @@ | |||
| export * from './tabs-config' | |||
There was a problem hiding this comment.
[maintainability]
Using export * can lead to unintentional exports and make it harder to track what is being exported from this module. Consider explicitly exporting only the necessary components to improve maintainability and clarity.
| @@ -0,0 +1,37 @@ | |||
| import _ from 'lodash' | |||
There was a problem hiding this comment.
[performance]
Consider importing only the specific lodash function needed (e.g., import { find } from 'lodash') to reduce bundle size and improve performance.
| isAnonymous: boolean, | ||
| isUnprivilegedUser: boolean, | ||
| ): string { | ||
| const matchItem = _.find(getTabsConfig( |
There was a problem hiding this comment.
[💡 readability]
The use of _.find here is correct, but since only one item is being checked, consider using Array.prototype.find directly for better readability and to avoid unnecessary dependency on lodash.
| .textTitle { | ||
| font-family: 'Inter', sans-serif; | ||
| color: var(--FontColor); | ||
| font-size: $sp-8; |
There was a problem hiding this comment.
[❗❗ correctness]
Ensure that $sp-8 is defined in the imported styles. If it's not defined, it could lead to unexpected behavior or errors.
| } | ||
|
|
||
| .blockExternalLink { | ||
| margin-left: $sp-2; |
There was a problem hiding this comment.
[❗❗ correctness]
Ensure that $sp-2 is defined in the imported styles. If it's not defined, it could lead to unexpected behavior or errors.
| <a | ||
| className={styles.blockExternalLink} | ||
| href={props.titleUrl} | ||
| target='_blank' |
There was a problem hiding this comment.
[❗❗ security]
Consider adding rel='noopener noreferrer' to the anchor tag to prevent potential security risks when opening links in a new tab.
| pageTitle: string | ||
| backUrl?: string | ||
| backAction?: () => void | ||
| titleUrl?: string | 'emptyLink' |
There was a problem hiding this comment.
[maintainability]
The use of the string literal 'emptyLink' for titleUrl seems unconventional and could lead to confusion. Consider using a more descriptive constant or refactoring to avoid this pattern.
|
|
||
| export const CustomerPortalAppContext: Context<CustomerPortalAppContextModel> | ||
| = createContext<CustomerPortalAppContextModel>({ | ||
| loginUserInfo: undefined, |
There was a problem hiding this comment.
[correctness]
Consider providing a more meaningful default value for loginUserInfo instead of undefined. This can help prevent potential runtime errors when accessing properties of loginUserInfo without checking for its existence.
| } from 'react' | ||
|
|
||
| import { tokenGetAsync, TokenModel } from '~/libs/core' | ||
| import { useOnComponentDidMount } from '~/apps/admin/src/lib/hooks' |
There was a problem hiding this comment.
[maintainability]
The useOnComponentDidMount hook is imported from the admin app's library. Ensure this hook is intended to be shared across different apps, as changes in the admin app could inadvertently affect the customer portal.
|
|
||
| useOnComponentDidMount(() => { | ||
| // get login user info on init | ||
| tokenGetAsync() |
There was a problem hiding this comment.
[❗❗ correctness]
Consider adding error handling for the tokenGetAsync call to manage potential failures in fetching the token, which could lead to undefined behavior if not handled.
| export const SWRConfigProvider: FC<PropsWithChildren> = props => ( | ||
| <SWRConfig | ||
| value={{ | ||
| fetcher: resource => xhrGetAsync(resource), |
There was a problem hiding this comment.
[❗❗ correctness]
Consider adding error handling to the fetcher function. If xhrGetAsync fails, it could lead to unhandled promise rejections or silent failures. Wrapping it in a try-catch block or handling errors in the promise chain would improve robustness.
| options?: AppNavigateOptions, | ||
| ) => { | ||
| if (typeof to === 'number') { | ||
| if (options?.fallback && to < 0 && location.key === 'default') { |
There was a problem hiding this comment.
[correctness]
The condition location.key === 'default' assumes that the default key is always 'default'. This assumption might not hold if the router's behavior changes or if the key is set differently. Consider making this check more robust or configurable.
| } | ||
|
|
||
| interface AppNavigateFunction { | ||
| (to: To | number, options?: AppNavigateOptions): void | Promise<void> |
There was a problem hiding this comment.
[maintainability]
The function signature for AppNavigateFunction is overloaded, but the second overload (delta: number) is not used in the implementation. Consider removing it if it's unnecessary or implementing its logic if intended.
| export interface BreadCrumbData { | ||
| index: number; | ||
| label: string | undefined; | ||
| path?: string | number; |
There was a problem hiding this comment.
[correctness]
The path property allows both string and number types. If path is intended to represent a URL or file path, consider restricting it to string for clarity and to prevent potential misuse.
| import { TokenModel } from '~/libs/core' | ||
|
|
||
| export interface CustomerPortalAppContextModel { | ||
| loginUserInfo: TokenModel | undefined |
There was a problem hiding this comment.
[💡 readability]
Consider using loginUserInfo?: TokenModel instead of loginUserInfo: TokenModel | undefined for optional properties in TypeScript. This improves readability and aligns with common TypeScript conventions.
| @@ -0,0 +1,193 @@ | |||
| @import '@libs/ui/styles/includes'; | |||
| @import url('https://fonts.googleapis.com/css2?family=Nunito+Sans:ital,opsz,wght@0,6..12,200..1000;1,6..12,200..1000&display=swap'); | |||
There was a problem hiding this comment.
[performance]
Importing fonts directly from Google Fonts using @import can lead to performance issues due to blocking rendering. Consider using a <link> tag in the HTML or a font loader to improve performance.
| border-radius: 0; | ||
| } | ||
| a { | ||
| color: #0d61bf; |
There was a problem hiding this comment.
[💡 maintainability]
The color #0d61bf is hardcoded here for links. Consider using the --Link variable for consistency and maintainability.
| line-height: 22px; | ||
| } | ||
| .select__value-container { | ||
| padding-left: $sp-4; |
There was a problem hiding this comment.
[correctness]
The use of $sp-4 for padding assumes that this variable is defined elsewhere. Ensure that all Sass variables used are defined and documented to avoid potential issues.
|
|
||
| .borderButton { | ||
| border: var(--ButtonColor) solid 1px; | ||
| border-radius: $sp-1; |
There was a problem hiding this comment.
[correctness]
The use of $sp-1 for border-radius assumes that this variable is defined elsewhere. Ensure that all Sass variables used are defined and documented to avoid potential issues.
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-3452
What's in this PR?