Added CustomErrorComponent, RoleBasedEmployeeHome, css enhancement #1407
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughWalkthroughThe changes involve updates to CSS stylesheets and enhancements to various components within the micro-ui-internals package. Key modifications include version updates for CSS files, the introduction of new hooks for tenant configuration, and improvements in error handling and user interface components. New classes and routes have been added to enhance visual presentation and user experience, while existing functionalities have been refined to support better data management and access control. Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (4)
micro-ui/web/micro-ui-internals/packages/modules/core/src/components/RoleBasedEmployeeHome.js (1)
91-102: Consider removing the commented code.The commented code block contains the previous implementation of rendering the module cards using the
EmployeeModuleCardcomponent. It is generally recommended to remove commented code from the codebase to improve code readability and maintainability.Apply this diff to remove the commented code:
- {/* {Object.keys(configEmployeeSideBar)?.map((current, index) => { - const moduleData = configEmployeeSideBar?.[current]; - const propsForModuleCard = { - // Icon: moduleData?.icon, - icon: "SupervisorAccount", - moduleName: t(moduleData?.label), - metrics: [], - links: moduleData.links, - }; - return <LandingPageCard buttonSize={"medium"} {...propsForModuleCard} />; - // return <EmployeeModuleCard {...propsForModuleCard} />; - })} */}micro-ui/web/micro-ui-internals/packages/libraries/src/services/molecules/Store/service.js (3)
66-67: Remove unnecessary commented-out codeThe commented-out code on lines 66-67 is no longer needed and can be removed to improve code readability.
Apply this diff:
- // throw new Error("Invalid URL") - // return;
Line range hint
98-144: RefactordigitInitDatato eliminate code duplicationThe
digitInitDatafunction contains duplicated logic in both the inlinefetchTenantConfigfunction and the defaultinitDataassignment. This redundancy can make the code harder to maintain and understand. Consider refactoring to consolidate theinitDataconstruction logic.Suggested approach:
- Merge the common parts of
initDataconstruction into a single function or block.- Utilize parameters to adjust values based on
tenantConfigFetchwithout duplicating code.- This refactoring will improve readability and maintainability.
175-178: Add braces toifstatement for clarityIt's recommended to use braces
{}around the code block following anifstatement, especially when it spans multiple lines, to enhance readability and prevent potential errors.Apply this diff:
-if (typeof moduleCode !== "string") - moduleCode.forEach((code) => { - moduleCodes.push(modulePrefix ? `${modulePrefix}-${code.toLowerCase()}` : `${code.toLowerCase()}`); - }); +if (typeof moduleCode !== "string") { + moduleCode.forEach((code) => { + moduleCodes.push(modulePrefix ? `${modulePrefix}-${code.toLowerCase()}` : `${code.toLowerCase()}`); + }); +}
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (7)
micro-ui/web/micro-ui-internals/example/package.jsonis excluded by!**/*.jsonmicro-ui/web/micro-ui-internals/package.jsonis excluded by!**/*.jsonmicro-ui/web/micro-ui-internals/packages/css/package.jsonis excluded by!**/*.jsonmicro-ui/web/micro-ui-internals/packages/modules/core/package.jsonis excluded by!**/*.jsonmicro-ui/web/micro-ui-internals/packages/modules/sandbox/package.jsonis excluded by!**/*.jsonmicro-ui/web/micro-ui-internals/packages/modules/utilities/package.jsonis excluded by!**/*.jsonmicro-ui/web/micro-ui-internals/packages/modules/workbench/package.jsonis excluded by!**/*.json
Files selected for processing (14)
- micro-ui/web/micro-ui-internals/example/public/index.html (1 hunks)
- micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/sandbox.scss (1 hunks)
- micro-ui/web/micro-ui-internals/packages/libraries/src/hooks/index.js (2 hunks)
- micro-ui/web/micro-ui-internals/packages/libraries/src/hooks/store.js (1 hunks)
- micro-ui/web/micro-ui-internals/packages/libraries/src/services/index.js (1 hunks)
- micro-ui/web/micro-ui-internals/packages/libraries/src/services/molecules/Store/service.js (4 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/core/src/App.js (2 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/core/src/components/CustomErrorComponent.js (1 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/core/src/components/RoleBasedEmployeeHome.js (3 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/configs/UICustomizations.js (1 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/applicationMgmt/ModuleMasterTable.js (1 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/applicationMgmt/SetupMaster.js (3 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/applicationMgmt/config/moduleMasterConfig.js (1 hunks)
- micro-ui/web/public/index.html (1 hunks)
Files skipped from review due to trivial changes (2)
- micro-ui/web/micro-ui-internals/example/public/index.html
- micro-ui/web/public/index.html
Additional context used
Path-based instructions (11)
micro-ui/web/micro-ui-internals/packages/libraries/src/hooks/index.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/libraries/src/hooks/store.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/libraries/src/services/index.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/libraries/src/services/molecules/Store/service.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/modules/core/src/App.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/modules/core/src/components/CustomErrorComponent.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/modules/core/src/components/RoleBasedEmployeeHome.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/modules/sandbox/src/configs/UICustomizations.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/applicationMgmt/ModuleMasterTable.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/applicationMgmt/SetupMaster.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/applicationMgmt/config/moduleMasterConfig.js (1)
Pattern
**/*.js: check
Biome
micro-ui/web/micro-ui-internals/packages/modules/core/src/components/RoleBasedEmployeeHome.js
[error] 85-85: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/configs/UICustomizations.js
[error] 240-261: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
Additional comments not posted (23)
micro-ui/web/micro-ui-internals/packages/libraries/src/services/index.js (2)
1-1: LGTM!The import statement is syntactically correct and the import path is valid. The addition of
useInitTenantConfigis consistent with the list of alterations provided.
3-3: LGTM!The export statement is syntactically correct. The addition of
useInitTenantConfigto the exports is consistent with the import statement and the list of alterations provided.micro-ui/web/micro-ui-internals/packages/libraries/src/hooks/store.js (1)
21-31: LGTM! The newuseInitTenantConfighook is a valuable addition.The introduction of the
useInitTenantConfighook enhances the modularity and reusability of the codebase by allowing tenant-specific configurations to be easily initialized and accessed. The hook is well-structured, follows best practices, and integrates seamlessly with the existing code patterns.Some key benefits of this hook include:
- Efficient data fetching and caching using
useQuerywith a unique key.- Prevention of unnecessary data refetching with an infinite stale time configuration.
- Conditional enabling based on the multi-root tenant feature, ensuring appropriate usage.
- Providing loading, error, and data states for consuming components to handle appropriately.
Overall, this is a great addition to the codebase!
micro-ui/web/micro-ui-internals/packages/modules/core/src/components/CustomErrorComponent.js (1)
8-47: LGTM!The
CustomErrorComponentis well-structured and follows best practices for React functional components. The use ofuseTranslationhook for internationalization and theuseLocationhook for retrieving the current location's state are good practices. The component uses a configuration object to define the error's visual elements, which is a good practice for maintainability. The use ofBackgroundwrapper andCardcomponent for layout ensures consistent and appealing presentation of error messages.Overall, the component enhances the application's error handling by providing a structured and localized way to inform users of issues, along with a clear action to take in response.
micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/applicationMgmt/ModuleMasterTable.js (1)
30-30: Verify the handling of the newmoduleparameter.The change in the URL construction logic to include the
moduleparameter looks good. It potentially enhances the specificity of the data being accessed or displayed based on the module context.Please ensure that the receiving component or route handler is updated to handle the new
moduleparameter correctly. You can use the following script to search for the usage of themoduleparameter:Verification successful
Verification successful:
moduleparameter is handled consistentlyThe codebase appears to be properly set up to handle the new
moduleparameter. Multiple components, includingSetupMaster.jsandModuleMasterTable.js, are using this parameter in their routing logic. Themodulecontext is being maintained across different views and passed to various routes, which aligns with the change we initially reviewed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for the usage of the `module` parameter in the codebase. # Test: Search for the `module` parameter usage in the route handlers and components. # Expect: Occurrences of the `module` parameter being accessed and used correctly. rg --type js $'module=\$\{module\}' -A 10Length of output: 7156
micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/applicationMgmt/config/moduleMasterConfig.js (1)
107-107: LGTM! The addition of themoduleproperty to theactionscolumn configuration object is a useful enhancement.This change allows the
actionscolumn to access themodulevalue directly from the configuration object, which may facilitate better handling or customization of actions associated with different modules in the application management context.micro-ui/web/micro-ui-internals/packages/modules/core/src/components/RoleBasedEmployeeHome.js (3)
2-2: LGTM!The import statement for the
LandingPageCardcomponent is correct.
51-60: LGTM!The logic for setting the
linkandiconproperties based on thelinkUrlis implemented correctly. The selected icons are appropriate for the corresponding keywords in thelinkUrl.
76-86: LGTM!The logic for creating the
childrenvariable by mapping over theconfigEmployeeSideBarobject is implemented correctly. TheLandingPageCardcomponent is used appropriately to render each module card with the necessary properties defined in thepropsForModuleCardobject.Tools
Biome
[error] 85-85: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
micro-ui/web/micro-ui-internals/packages/modules/core/src/App.js (2)
8-8: LGTM!The import statement for
CustomErrorComponentis syntactically correct and aligns with the PR objective of handling invalid URLs.
102-104: Great work on adding the route for handling invalid URLs!The new route is defined correctly and aligns perfectly with the PR objective. It enhances the application's routing capabilities and improves the user experience by providing a dedicated error component for invalid URLs.
The route integrates seamlessly into the existing routing structure, making it a valuable addition to the codebase.
micro-ui/web/micro-ui-internals/packages/libraries/src/hooks/index.js (2)
7-7: LGTM!The import statement for the newly added
useInitTenantConfighook is syntactically correct. This hook will now be available for consumption by other modules that import from this file.
170-170: Verify the implementation and test the newly exported hook.The export statement for the newly added
useInitTenantConfighook is syntactically correct. This hook will now be available for consumption by other modules that import theHooksobject.However, it's crucial to ensure that the
useInitTenantConfighook is implemented correctly in the./storemodule and thoroughly tested before consuming it in other modules to avoid any potential issues.micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/sandbox.scss (6)
341-352: LGTM!The styles for error alerts look good. Using a red color for the alert heading effectively distinguishes error messages from regular alerts and aligns with common UI conventions.
354-356: Verify the fixed width in the context of the overall design.Setting a fixed width for error cards is a good approach to ensure consistent sizing and layout. The chosen width of
35remseems reasonable, but please verify that it aligns well with the overall design and layout of the application.
357-359: LGTM!Centering the button horizontally using
margin: autois a simple and effective approach for error messages or alerts. The styles look good.
360-363: LGTM!Using flexbox with a column direction is a good approach for vertically stacking elements within the employee card on the landing page. It allows for easy alignment and spacing of the card's content. The styles look good.
364-366: LGTM!Centering the text within the card header using
text-align: centeris a good approach to improve readability and visual balance. The styles look good.
367-369: LGTM!Centering the text within the card using
text-align: centeris a good approach to improve readability and visual consistency. The styles look good.micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/applicationMgmt/SetupMaster.js (3)
14-14: LGTM!The
useAccessControlhook is a great addition to manage user permissions dynamically. Therefetchfunction can be used to ensure that the latest access control state is always reflected in the UI.
67-67: LGTM!Calling
refetch()in theonSuccesscallback is a good practice to ensure that the access control data is refreshed after a successful operation. This improves the responsiveness of the application to user actions.
167-167: LGTM!The conditional class assignment for the
PopUpcomponent based on theshowPopUpstate is a nice touch. It allows for more nuanced styling of the popup and enhances the user experience by visually distinguishing between different types of messages.micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/configs/UICustomizations.js (1)
240-261: LGTM!The changes to the
handleRedirectfunction improve the functionality and robustness of the redirect logic. The addition of themodulequery parameter provides more context to the redirected page. The changes do not introduce any new issues or bugs.The static analysis hint about wrapping the function declaration in a block is a false positive because the function is already wrapped in a block (the
additionalCustomizationsfunction). The function is not accessible outside the switch clause.Also applies to: 264-264
Tools
Biome
[error] 240-261: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
| const ModuleBasedErrorConfig = { | ||
| sandbox: { | ||
| imgUrl: `https://s3.ap-south-1.amazonaws.com/egov-qa-assets/error-image.png`, | ||
| infoHeader: "WRONG_TENANT_SIGN_UP", | ||
| infoMessage: "WRONG_TENANT_SIGN_UP_MESSAGE", | ||
| buttonInfo: "WRONG_TENANT_SIGN_UP_BUTTON", | ||
| action: () => { | ||
| history.push(`/${window.globalPath}/`); | ||
| }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Consider making the error handling more generic to improve reusability.
The error handling in the CustomErrorComponent is specific to a "sandbox" module, which might limit its reusability in other parts of the application. Consider making the error handling more generic by allowing the configuration object to be passed as a prop to the component. This way, the component can be reused for different types of errors across the application.
| return <LandingPageCard buttonSize={"medium"} {...propsForModuleCard} />; | ||
| // return <EmployeeModuleCard {...propsForModuleCard} />; | ||
| })} */} | ||
| {React.Children.map(children, (child) => React.cloneElement(child))} |
There was a problem hiding this comment.
Add a key property to the elements in the iterable.
When rendering elements in an iterable using React.Children.map, it is recommended to provide a unique key property to each element. The key helps React efficiently update and reorder the list if needed.
Apply this diff to add a key property to the elements:
-{React.Children.map(children, (child) => React.cloneElement(child))}
+{React.Children.map(children, (child, index) => React.cloneElement(child, { key: index }))}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {React.Children.map(children, (child) => React.cloneElement(child))} | |
| {React.Children.map(children, (child, index) => React.cloneElement(child, { key: index }))} |
| modules: | ||
| typeof moduleCode == "string" | ||
| ? modulePrefix | ||
| ? [`${modulePrefix}-${moduleCode.toLowerCase()}`] | ||
| : [`${moduleCode.toLowerCase()}`] | ||
| : moduleCodes, |
There was a problem hiding this comment.
Simplify nested ternary operators for better readability
The nested ternary operators in lines 181-185 make the code difficult to read and maintain. Consider refactoring the logic using clearer conditional statements.
Refactored code:
let modulesList;
if (typeof moduleCode === "string") {
if (modulePrefix) {
modulesList = [`${modulePrefix}-${moduleCode.toLowerCase()}`];
} else {
modulesList = [`${moduleCode.toLowerCase()}`];
}
} else {
modulesList = moduleCodes;
}
const LocalePromise = LocalizationService.getLocale({
modules: modulesList,
locale: language,
tenantId: stateCode,
});This approach improves readability and makes future maintenance easier.
| if (tenantConfigFetch) { | ||
| const tenantConfig = await StoreService.getTenantConfig(stateCode, enabledModules); // Await the async call | ||
| } |
There was a problem hiding this comment.
Remove redundant call to StoreService.getTenantConfig
The call to StoreService.getTenantConfig on line 99 is redundant because its result is not utilized. Additionally, getTenantConfig is called again inside fetchTenantConfig(), leading to an unnecessary API call.
Apply this diff to remove the redundant call:
-if (tenantConfigFetch) {
- const tenantConfig = await StoreService.getTenantConfig(stateCode, enabledModules); // Await the async call
-}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (tenantConfigFetch) { | |
| const tenantConfig = await StoreService.getTenantConfig(stateCode, enabledModules); // Await the async call | |
| } |
micro-ui/web/micro-ui-internals/packages/libraries/src/services/molecules/Store/service.js
Show resolved
Hide resolved
No description provided.