[WEB-2106] fix: add date and state change functionalities to list and grid view#5533
Conversation
WalkthroughThe changes involve significant updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ModuleCardItem
participant ModuleListItemAction
participant ModuleStatusDropdown
participant useModule
User->>ModuleCardItem: Select date range
ModuleCardItem->>useModule: updateModuleDetails(dateRange)
useModule-->>ModuleCardItem: Success/Error notification
ModuleCardItem->>User: Display notification
User->>ModuleListItemAction: Change module status
ModuleListItemAction->>useModule: updateModuleDetails(newStatus)
useModule-->>ModuleListItemAction: Success/Error notification
ModuleListItemAction->>User: Display notification
User->>ModuleStatusDropdown: Select new status
ModuleStatusDropdown->>useModule: updateModuleDetails(newStatus)
useModule-->>ModuleStatusDropdown: Success/Error notification
ModuleStatusDropdown->>User: Display notification
Poem
Tip New featuresWalkthrough comment now includes:
Notes:
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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- web/core/components/modules/module-card-item.tsx (9 hunks)
- web/core/components/modules/module-list-item-action.tsx (5 hunks)
- web/core/components/modules/module-status-dropdown.tsx (1 hunks)
Additional context used
Biome
web/core/components/modules/module-list-item-action.tsx
[error] 49-49: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
web/core/components/modules/module-card-item.tsx
[error] 57-57: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
Additional comments not posted (4)
web/core/components/modules/module-list-item-action.tsx (2)
20-21: Approved: New imports for enhanced functionality.The addition of
ModuleStatusSelectionandDateRangeDropdownaligns with the new functionalities introduced in this PR. These components are essential for the date and status selection features.Also applies to: 23-24
136-156: Approved: Integration ofDateRangeDropdownandModuleStatusSelection.The integration of
DateRangeDropdownandModuleStatusSelectionis done effectively, enhancing the user experience by allowing direct interactions with module dates and statuses from the list view.Also applies to: 159-163
web/core/components/modules/module-card-item.tsx (2)
28-29: Approved: New imports for enhanced functionality.The addition of
ModuleStatusSelectionandDateRangeDropdownaligns with the new functionalities introduced in this PR. These components are essential for the date and status selection features.
215-219: Approved: Integration ofDateRangeDropdownandModuleStatusSelection.The integration of
DateRangeDropdownandModuleStatusSelectionis done effectively, enhancing the user experience by allowing direct interactions with module dates and statuses from the grid view.Also applies to: 244-265
| export const ModuleStatusSelection : FC<Props> = observer((props : Props) => { | ||
| const {isDisabled, moduleDetails, handleModuleDetailsChange} = props; | ||
| const moduleStatus = MODULE_STATUS.find((status) => status.value === moduleDetails.status); | ||
|
|
||
| if(!moduleStatus) return <></> | ||
|
|
||
| return ( | ||
| <CustomSelect | ||
| customButton={ | ||
| <span | ||
| className={`flex h-6 w-20 items-center justify-center rounded-sm text-center text-xs ${ | ||
| isDisabled ? "cursor-not-allowed" : "cursor-pointer" | ||
| }`} | ||
| style={{ | ||
| color: moduleStatus ? moduleStatus.color : "#a3a3a2", | ||
| backgroundColor: moduleStatus ? `${moduleStatus.color}20` : "#a3a3a220", | ||
| }} | ||
| > | ||
| {moduleStatus?.label ?? "Backlog"} | ||
| </span> | ||
| } | ||
| value={moduleStatus?.value} | ||
| onChange={(val: TModuleStatus)=>{ | ||
| handleModuleDetailsChange({status: val}) | ||
| }} | ||
| disabled={isDisabled} | ||
| > | ||
| {MODULE_STATUS.map((status) => ( | ||
| <CustomSelect.Option key={status.value} value={status.value}> | ||
| <div className="flex items-center gap-2"> | ||
| <ModuleStatusIcon status={status.value} /> | ||
| {status.label} | ||
| </div> | ||
| </CustomSelect.Option> | ||
| ))} | ||
| </CustomSelect> | ||
| ) | ||
| }) No newline at end of file |
There was a problem hiding this comment.
Well-implemented module status selection component.
The ModuleStatusSelection component is well-implemented with appropriate handling of props and state. The use of MobX's observer for reactivity and the CustomSelect component for user interaction are correctly applied. The early return for an undefined status and the dynamic rendering based on the isDisabled state are good practices.
However, consider adding a comment explaining the use of the magic string "#a3a3a220" for default background color in the inline style, as it might not be immediately clear to other developers why this specific value is used.
Consider adding a comment for the magic string used in the background color:
+ // Default light grey background color when no status is found
backgroundColor: moduleStatus ? `${moduleStatus.color}20` : "#a3a3a220",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.
| export const ModuleStatusSelection : FC<Props> = observer((props : Props) => { | |
| const {isDisabled, moduleDetails, handleModuleDetailsChange} = props; | |
| const moduleStatus = MODULE_STATUS.find((status) => status.value === moduleDetails.status); | |
| if(!moduleStatus) return <></> | |
| return ( | |
| <CustomSelect | |
| customButton={ | |
| <span | |
| className={`flex h-6 w-20 items-center justify-center rounded-sm text-center text-xs ${ | |
| isDisabled ? "cursor-not-allowed" : "cursor-pointer" | |
| }`} | |
| style={{ | |
| color: moduleStatus ? moduleStatus.color : "#a3a3a2", | |
| backgroundColor: moduleStatus ? `${moduleStatus.color}20` : "#a3a3a220", | |
| }} | |
| > | |
| {moduleStatus?.label ?? "Backlog"} | |
| </span> | |
| } | |
| value={moduleStatus?.value} | |
| onChange={(val: TModuleStatus)=>{ | |
| handleModuleDetailsChange({status: val}) | |
| }} | |
| disabled={isDisabled} | |
| > | |
| {MODULE_STATUS.map((status) => ( | |
| <CustomSelect.Option key={status.value} value={status.value}> | |
| <div className="flex items-center gap-2"> | |
| <ModuleStatusIcon status={status.value} /> | |
| {status.label} | |
| </div> | |
| </CustomSelect.Option> | |
| ))} | |
| </CustomSelect> | |
| ) | |
| }) | |
| export const ModuleStatusSelection : FC<Props> = observer((props : Props) => { | |
| const {isDisabled, moduleDetails, handleModuleDetailsChange} = props; | |
| const moduleStatus = MODULE_STATUS.find((status) => status.value === moduleDetails.status); | |
| if(!moduleStatus) return <></> | |
| return ( | |
| <CustomSelect | |
| customButton={ | |
| <span | |
| className={`flex h-6 w-20 items-center justify-center rounded-sm text-center text-xs ${ | |
| isDisabled ? "cursor-not-allowed" : "cursor-pointer" | |
| }`} | |
| style={{ | |
| color: moduleStatus ? moduleStatus.color : "#a3a3a2", | |
| // Default light grey background color when no status is found | |
| backgroundColor: moduleStatus ? `${moduleStatus.color}20` : "#a3a3a220", | |
| }} | |
| > | |
| {moduleStatus?.label ?? "Backlog"} | |
| </span> | |
| } | |
| value={moduleStatus?.value} | |
| onChange={(val: TModuleStatus)=>{ | |
| handleModuleDetailsChange({status: val}) | |
| }} | |
| disabled={isDisabled} | |
| > | |
| {MODULE_STATUS.map((status) => ( | |
| <CustomSelect.Option key={status.value} value={status.value}> | |
| <div className="flex items-center gap-2"> | |
| <ModuleStatusIcon status={status.value} /> | |
| {status.label} | |
| </div> | |
| </CustomSelect.Option> | |
| ))} | |
| </CustomSelect> | |
| ) | |
| }) |
| const handleModuleDetailsChange = async (payload: Partial<IModule>) => { | ||
| if (!workspaceSlug || !projectId) return; | ||
|
|
||
| await updateModuleDetails(workspaceSlug.toString(), projectId.toString(), moduleId, payload) | ||
| .then((res) => { | ||
| setToast({ | ||
| type: TOAST_TYPE.SUCCESS, | ||
| title: "Success!", | ||
| message: "Module updated successfully.", | ||
| }); | ||
| }) | ||
| .catch((err) => { | ||
| setToast({ | ||
| type: TOAST_TYPE.ERROR, | ||
| title: "Error!", | ||
| message: err?.detail ?? "Module could not be updated. Please try again.", | ||
| }); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Approved: Asynchronous handling of module details.
The handleModuleDetailsChange function is well-implemented, providing robust error handling and user feedback through toast notifications. However, consider simplifying the boolean expression in line 49 as suggested by static analysis to enhance code readability.
Consider simplifying the boolean expression in line 49:
- const isDisabled = !isEditingAllowed || (moduleDetails.archived_at ? true : false);
+ const isDisabled = !isEditingAllowed || !!moduleDetails.archived_at;Committable suggestion was skipped due to low confidence.
| const moduleStatus = MODULE_STATUS.find((status) => status.value === moduleDetails.status); | ||
|
|
||
| const isEditingAllowed = !!currentProjectRole && currentProjectRole >= EUserProjectRoles.MEMBER; | ||
| const isDisabled = !isEditingAllowed || (moduleDetails.archived_at ? true : false) ; |
There was a problem hiding this comment.
Refactor suggestion: Simplify boolean expression.
The isDisabled variable can be simplified to enhance code readability and maintainability.
Apply this change to simplify the boolean expression:
- const isDisabled = !isEditingAllowed || (moduleDetails.archived_at ? true : false);
+ const isDisabled = !isEditingAllowed || !!moduleDetails.archived_at;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.
| const isDisabled = !isEditingAllowed || (moduleDetails.archived_at ? true : false) ; | |
| const isDisabled = !isEditingAllowed || !!moduleDetails.archived_at; |
Tools
Biome
[error] 49-49: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
There was a problem hiding this comment.
Please check this as well
There was a problem hiding this comment.
@rahulramesha, I've reviewed the refactor suggestion, and it is indeed correct. The ternary operator in the original code is unnecessary, and the simplified expression using !!moduleDetails.archived_at is more concise while maintaining the same functionality. You can safely apply this change to improve code readability.
| const handleModuleDetailsChange = async (payload: Partial<IModule>) => { | ||
| if (!workspaceSlug || !projectId) return; | ||
|
|
||
| await updateModuleDetails(workspaceSlug.toString(), projectId.toString(), moduleId, payload) | ||
| .then((res) => { | ||
| setToast({ | ||
| type: TOAST_TYPE.SUCCESS, | ||
| title: "Success!", | ||
| message: "Module updated successfully.", | ||
| }); | ||
| }) | ||
| .catch((err) => { | ||
| setToast({ | ||
| type: TOAST_TYPE.ERROR, | ||
| title: "Error!", | ||
| message: err?.detail ?? "Module could not be updated. Please try again.", | ||
| }); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Approved: Asynchronous handling of module details.
The handleModuleDetailsChange function is well-implemented, providing robust error handling and user feedback through toast notifications. However, consider simplifying the boolean expression in line 57 as suggested by static analysis to enhance code readability.
Consider simplifying the boolean expression in line 57:
- const isDisabled = !isEditingAllowed || (moduleDetails?.archived_at ? true : false);
+ const isDisabled = !isEditingAllowed || !!moduleDetails?.archived_at;Committable suggestion was skipped due to low confidence.
| // derived values | ||
| const moduleDetails = getModuleById(moduleId); | ||
| const isEditingAllowed = !!currentProjectRole && currentProjectRole >= EUserProjectRoles.MEMBER; | ||
| const isDisabled = !isEditingAllowed || (moduleDetails?.archived_at ? true : false) ; |
There was a problem hiding this comment.
Refactor suggestion: Simplify boolean expression.
The isDisabled variable can be simplified to enhance code readability and maintainability.
Apply this change to simplify the boolean expression:
- const isDisabled = !isEditingAllowed || (moduleDetails?.archived_at ? true : false);
+ const isDisabled = !isEditingAllowed || !!moduleDetails?.archived_at;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.
| const isDisabled = !isEditingAllowed || (moduleDetails?.archived_at ? true : false) ; | |
| const isDisabled = !isEditingAllowed || !!moduleDetails?.archived_at; |
Tools
Biome
[error] 57-57: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| // derived values | ||
| const moduleDetails = getModuleById(moduleId); | ||
| const isEditingAllowed = !!currentProjectRole && currentProjectRole >= EUserProjectRoles.MEMBER; | ||
| const isDisabled = !isEditingAllowed || (moduleDetails?.archived_at ? true : false) ; |
| // hooks | ||
| import { useEventTracker, useMember, useModule, useUser } from "@/hooks/store"; | ||
| import { ButtonAvatars } from "../dropdowns/member/avatar"; | ||
| import { ModuleStatusSelection } from "@/components/modules/module-status-dropdown"; |
There was a problem hiding this comment.
can you run yarn lint and fix them in your PR. This would have thrown a lint error
There was a problem hiding this comment.
- Fixed the lint error
- Improved the logic for isDisabled
| const moduleStatus = MODULE_STATUS.find((status) => status.value === moduleDetails.status); | ||
|
|
||
| const isEditingAllowed = !!currentProjectRole && currentProjectRole >= EUserProjectRoles.MEMBER; | ||
| const isDisabled = !isEditingAllowed || (moduleDetails.archived_at ? true : false) ; |
There was a problem hiding this comment.
Please check this as well
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- web/core/components/modules/module-card-item.tsx (8 hunks)
- web/core/components/modules/module-list-item-action.tsx (5 hunks)
Files skipped from review as they are similar to previous changes (2)
- web/core/components/modules/module-card-item.tsx
- web/core/components/modules/module-list-item-action.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (2)
web/core/components/modules/module-list-item-action.tsx (2)
109-127: Approved: Asynchronous handling of module details.The
handleModuleDetailsChangefunction is well-implemented, providing robust error handling and user feedback through toast notifications. Consider adding more detailed error logging for debugging purposes.
133-160: Approved: Rendering logic and conditional rendering.The conditional rendering based on user permissions and module state is well-implemented. Consider reviewing accessibility features to ensure that all users can interact with these components effectively.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- web/core/components/modules/module-card-item.tsx (7 hunks)
- web/core/components/modules/module-list-item-action.tsx (4 hunks)
- web/core/components/modules/module-status-dropdown.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- web/core/components/modules/module-status-dropdown.tsx
Additional comments not posted (7)
web/core/components/modules/module-list-item-action.tsx (2)
Line range hint
1-21: Approved: Component setup and imports.The setup and imports are appropriate for the functionality of the component, including reactive state management and routing.
39-48: Approved: Use of hooks and initial component setup.The use of React and MobX hooks for state management and routing is correctly implemented and follows best practices.
web/core/components/modules/module-card-item.tsx (5)
3-22: Approved: Import statements are correctly organized.The import statements are well-organized and relevant to the component's functionality, including the new features introduced in this PR.
Line range hint
24-48: Approved: Component setup and initial hook usage.The setup of the
ModuleCardItemcomponent is standard and appropriate, utilizing MobX for reactive state management and multiple custom hooks for data fetching and management.
56-56: Refactor suggestion: Simplify boolean expression.The
isDisabledvariable can be simplified to enhance code readability and maintainability.Apply this change to simplify the boolean expression:
- const isDisabled = !isEditingAllowed || (moduleDetails?.archived_at ? true : false); + const isDisabled = !isEditingAllowed || !!moduleDetails?.archived_at;
124-142: Approved: Asynchronous handling of module details.The
handleModuleDetailsChangefunction is well-implemented, providing robust error handling and user feedback through toast notifications. This aligns with best practices for asynchronous operations in React components.
Line range hint
212-264: Approved: Integration ofDateRangeDropdownandModuleStatusDropdown.The integration of
DateRangeDropdownandModuleStatusDropdowninto theModuleCardItemcomponent enhances user interactivity by allowing direct manipulation of module details. The conditional rendering based on theisDisabledstate ensures that these functionalities are accessible appropriately based on user permissions and module status.
| const moduleStatus = MODULE_STATUS.find((status) => status.value === moduleDetails.status); | ||
|
|
||
| const isEditingAllowed = !!currentProjectRole && currentProjectRole >= EUserProjectRoles.MEMBER; | ||
| const isDisabled = !isEditingAllowed || !!moduleDetails?.archived_at; |
There was a problem hiding this comment.
Confirmed issue resolution: Simplified boolean expression.
The boolean expression for isDisabled has been simplified, enhancing readability and maintainability. This change correctly implements previous suggestions.
3 participants
[WEB-2106]
This PR introduces the ability to change the state and date of Module from list and grid view.
Previously possible via edit and info button only.
All the styling is made similar to the date picker present in the cycles view.
Changes:
LIST VIEW:
GRID VIEW:
STYLING DETAILS:
LIST VIEW:
GRID VIEW:
CYCLES:
ARCHIVED MODULES:
Screen.Recording.2024-09-05.at.7.02.23.PM.mov
Reference:
[WEB-2106]
Summary by CodeRabbit
DateRangeDropdownfor selecting start and end dates for modules, improving user interaction.ModuleStatusDropdowncomponent for managing module statuses with enhanced visual cues.