-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
side bar #1384
side bar #1384
Conversation
WalkthroughWalkthroughThe changes introduced in this pull request enhance navigation functionality within a multi-root tenant environment. Key modifications include the implementation of logic to dynamically adjust URLs in the 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- micro-ui/web/micro-ui-internals/packages/modules/core/src/components/TopBarSideBar/SideBar/EmployeeSideBar.js (3 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/hrms/src/components/pageComponents/jurisdiction.js (2 hunks)
Additional context used
Path-based instructions (2)
micro-ui/web/micro-ui-internals/packages/modules/core/src/components/TopBarSideBar/SideBar/EmployeeSideBar.js (1)
Pattern
**/*.js
: checkmicro-ui/web/micro-ui-internals/packages/modules/hrms/src/components/pageComponents/jurisdiction.js (1)
Pattern
**/*.js
: check
Additional comments not posted (5)
micro-ui/web/micro-ui-internals/packages/modules/core/src/components/TopBarSideBar/SideBar/EmployeeSideBar.js (3)
11-11
: LGTM!The introduction of the
isMultiRootTenant
constant aligns with the PR objective of handling navigation URLs based on multi-root tenant environment. The usage of the constant is correct.
14-14
: LGTM!Retrieving the current tenant ID using
Digit.ULBService.getCurrentTenantId()
aligns with the PR objective of handling navigation URLs based on multi-root tenant environment. The usage of the tenant ID is correct.
89-103
: LGTM!The modifications to the
navigateToRespectiveURL
function align with the PR objective of handling navigation URLs based on multi-root tenant environment. The logic for adjusting the navigation URL is correct and handles both multi-root tenant and non-multi-root tenant scenarios appropriately. The code changes are well-structured and easy to understand.micro-ui/web/micro-ui-internals/packages/modules/hrms/src/components/pageComponents/jurisdiction.js (2)
197-197
: LGTM!The change to the
useEffect
hook's dependency onsubTenantList
enhances the control flow by ensuring that the selection of boundaries is contingent upon the presence of a multi-root tenant. This potentially improves the component's responsiveness to tenant changes and aligns with the PR objective of adding an employee side bar.
280-280
: LGTM!The modification to the
optionKey
prop allows for more flexible handling of options based on the tenant context. It dynamically determines the value based on the multi-root tenant condition, which could affect how data is displayed or interacted with in the UI. This change aligns with the PR objective of adding an employee side bar and enhances the component's adaptability to different tenant configurations.
@@ -2,13 +2,16 @@ import React, { useRef, useEffect, useState } from "react"; | |||
import { Sidebar, Loader } from "@egovernments/digit-ui-components"; | |||
import { useTranslation } from "react-i18next"; | |||
import { useHistory } from "react-router-dom"; | |||
import { update } from "lodash"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unused import.
The update
function imported from the lodash
library is not used anywhere in the file. Please remove this import to avoid unnecessary bundle size and potential performance issues.
Apply this diff to remove the unused import:
-import { update } from "lodash";
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.
import { update } from "lodash"; |
else{ | ||
updatedUrl = DIGIT_UI_CONTEXTS?.every((e) => url?.indexOf(`/${e}`) === -1) ? hostUrl + "/employee/" + url : hostUrl + url; | ||
} | ||
window.location.href = updatedUrl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we shouldnt do this within sandbox app
history ,push is recommended
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mithunhegde-egov the logic should be written for else section this section is working fine
const { t } = useTranslation(); | ||
const history = useHistory(); | ||
const tenantId = Digit.ULBService.getCurrentTenantId(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use stateid not current tenant id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mithunhegde-egov please check |
employee side bar checks added for navigation.