[WEB-2213] fix: group by persistence for list view #5590
Conversation
WalkthroughThe changes introduce enhancements across several components, focusing on managing issue grouping and filtering in the Kanban layout. Key functionalities include the addition of Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
sharma01ketan
changed the title
| @@ -107,6 +112,21 @@ export const BaseListRoot = observer((props: IBaseListRoot) => { | |||
| [fetchNextIssues] | |||
| ); | |||
|
|
|||
| //kanbanFilters and EIssueFilterType.KANBAN_FILTERS are used becuase the state is shared between kanban view and list view | |||
| const handleCollapsedGroups = (toggle: "group_by" | "sub_group_by", value: string) => { | |||
There was a problem hiding this comment.
please wrap this with useCallback and add necessary dependencies.
Also for list view, it doesn't need the first variable, since it is always group_by
There was a problem hiding this comment.
Made the necessary changes:
- Removed sub_group_by property from list view
- Used useCallback for both list view and kanban view's filter handling function
- Made naming consistent, Renamed kanbanFilters -> collapsedGroups and handleKanbanFilters -> handleCollapsedGroups for both list and kanban view
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (12)
web/core/components/issues/issue-layouts/kanban/headers/sub-group-by-card.tsx (1)
12-13: LGTM! Consider updating the type name for clarity.The renaming from
kanbanFilterstocollapsedGroupsandhandleKanbanFilterstohandleCollapsedGroupsaligns well with the PR objectives of focusing on grouping persistence. The changes are consistent and maintain the existing type structure.Consider updating the type name
TIssueKanbanFiltersto reflect the new terminology, e.g.,TIssueCollapsedGroups. This would make the code more self-explanatory and consistent with the new naming convention.web/core/components/issues/issue-layouts/list/default.tsx (2)
51-52: LGTM: New properties added to IList interface.The addition of
handleCollapsedGroupsandcollapsedGroupsto theIListinterface aligns with the PR objectives of implementing persistence for groupings and sharing state with the Kanban view.Consider making the
toggleparameter ofhandleCollapsedGroupsmore flexible for future use:handleCollapsedGroups: (toggle: string, value: string) => void;This would allow for easier extension if other toggle types are needed in the future.
Line range hint
136-171: LGTM: New props correctly passed to ListGroup.The addition of
handleCollapsedGroupsandcollapsedGroupsprops to theListGroupcomponent is correct and consistent with the changes made to theIListinterface andListcomponent.There's a trailing space on line 136. Please remove it to resolve the linting error:
+136web/core/components/issues/issue-layouts/list/headers/group-by-card.tsx (1)
Line range hint
1-168: Consider simplifying state management in future refactoring.While the current implementation works correctly, there's an opportunity to simplify the component's state management in the future. The component currently uses a mix of local state (
useState), props, and potentially global state (via MobX observer). Consider the following suggestions for future refactoring:
- Move more logic to the parent component or a central state management solution to reduce the complexity of this component.
- Evaluate if some of the local state (
isOpen,openExistingIssueListModal) could be managed by the parent component to improve the overall state flow.- Consider using custom hooks to encapsulate some of the logic and state management, which could improve reusability and testability.
These suggestions are not critical for the current PR but could be considered for future improvements to enhance maintainability and scalability of the codebase.
web/core/components/issues/issue-layouts/kanban/headers/group-by-card.tsx (1)
Line range hint
1-186: Consider broader impact of component changes.While the changes to
HeaderGroupByCardimprove its implementation, they may have implications for other parts of the application:
- Parent components that use
HeaderGroupByCardwill need to be updated to provide the newcollapsedGroupsprop instead ofkanbanFilters.- The logic for managing collapsed groups may have moved to a different part of the application, potentially affecting state management.
- Other components that previously interacted with
kanbanFiltersmight need to be updated to work withcollapsedGroups.To ensure a smooth transition:
- Review and update all usage of
HeaderGroupByCardthroughout the application.- Verify that the state management for collapsed groups is consistent across different views (e.g., List View and Kanban View).
- Update any relevant documentation or comments to reflect the new prop names and functionality.
- Consider adding a brief comment in the component explaining the purpose of
collapsedGroupsand how it differs from the previouskanbanFilters.web/core/components/issues/issue-layouts/kanban/default.tsx (2)
45-46: LGTM! Consider renaming the handler function for clarity.The addition of
collapsedGroupsandhandleCollapsedGroupsto theIKanBaninterface aligns well with the PR objectives of implementing state persistence for groupings. The types are well-defined, providing type safety and clarity.Consider renaming
handleCollapsedGroupstotoggleCollapsedGroupsto more accurately reflect its functionality. This would make the purpose of the function more immediately clear to other developers.- handleCollapsedGroups: (toggle: "group_by" | "sub_group_by", value: string) => void; + toggleCollapsedGroups: (toggle: "group_by" | "sub_group_by", value: string) => void;
130-130: LGTM! Consider extracting the condition for better readability.The update to use
collapsedGroups?.group_by.includes(_list.id)is correct and aligns with the newcollapsedGroupsproperty. The logic accurately determines whether to show or hide issues based on the collapsed state.To improve readability, consider extracting the condition into a separate variable with a descriptive name. This would make the code more self-documenting:
- if (collapsedGroups?.group_by.includes(_list.id)) groupVisibility.showIssues = false; + const isGroupCollapsed = collapsedGroups?.group_by.includes(_list.id); + if (isGroupCollapsed) groupVisibility.showIssues = false;web/core/components/issues/issue-layouts/kanban/base-kanban-root.tsx (2)
209-224: LGTM! Consider a minor enhancement for clarity.The
handleCollapsedGroupsfunction effectively implements the toggling logic for collapsing/expanding groups, aligning with the PR objective of implementing state persistence for groupings. The use ofuseCallbackis appropriate for performance optimization.Consider renaming the
collapsedGroupsvariable within the function toupdatedCollapsedGroupsfor better clarity:- let collapsedGroups = issuesFilter?.issueFilters?.kanbanFilters?.[toggle] || []; + let updatedCollapsedGroups = issuesFilter?.issueFilters?.kanbanFilters?.[toggle] || []; - if (collapsedGroups.includes(value)) { - collapsedGroups = collapsedGroups.filter((_value) => _value != value); + if (updatedCollapsedGroups.includes(value)) { + updatedCollapsedGroups = updatedCollapsedGroups.filter((_value) => _value != value); } else { - collapsedGroups.push(value); + updatedCollapsedGroups.push(value); } updateFilters(projectId?.toString() ?? "", EIssueFilterType.KANBAN_FILTERS, { - [toggle]: collapsedGroups, + [toggle]: updatedCollapsedGroups, });This change would help distinguish between the input
collapsedGroupsand the updated version within the function.
271-272: LGTM! Props added for collapsed groups management.The addition of
handleCollapsedGroupsandcollapsedGroupsprops to the KanBanView component allows for proper management of the collapsed state of groups and subgroups, aligning with the PR objective of implementing state persistence for groupings.For consistency with other props, consider using object shorthand notation:
- handleCollapsedGroups={handleCollapsedGroups} - collapsedGroups={collapsedGroups} + {handleCollapsedGroups} + {collapsedGroups}This change would make the prop passing style consistent with other props in the component.
web/core/components/issues/issue-layouts/list/list-group.tsx (2)
95-95: LGTM: Deriving isExpanded state from collapsedGroups.The
isExpandedstate is now correctly derived from thecollapsedGroupsprop, which aligns with the PR objectives. This change ensures that the expansion state is consistent with the persisted groupings.Consider improving readability by inverting the logic:
const isExpanded = !collapsedGroups?.group_by.includes(group.id);to:
const isExpanded = collapsedGroups?.group_by.includes(group.id) !== true;This makes the logic more explicit and easier to understand at a glance.
215-218: LGTM: Automatic group expansion on drop.The addition of logic to automatically expand a collapsed group when an item is dropped into it improves the user experience and aligns with the PR objectives.
For consistency with the rest of the codebase, consider using strict equality and adding a space after
if:if (!isExpanded) { handleCollapsedGroups("group_by", group.id); }web/core/components/issues/issue-layouts/list/base-list-root.tsx (1)
115-115: Typo in comment: 'becuase' should be 'because'There's a typo in the comment at line 115. The word 'becuase' should be corrected to 'because'.
Apply this diff to fix the typo:
-// kanbanFilters and EIssueFilterType.KANBAN_FILTERS are used becuase the state is shared between kanban view and list view +// kanbanFilters and EIssueFilterType.KANBAN_FILTERS are used because the state is shared between kanban view and list view
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- web/core/components/issues/issue-layouts/kanban/base-kanban-root.tsx (2 hunks)
- web/core/components/issues/issue-layouts/kanban/default.tsx (4 hunks)
- web/core/components/issues/issue-layouts/kanban/headers/group-by-card.tsx (3 hunks)
- web/core/components/issues/issue-layouts/kanban/headers/sub-group-by-card.tsx (1 hunks)
- web/core/components/issues/issue-layouts/kanban/swimlanes.tsx (12 hunks)
- web/core/components/issues/issue-layouts/list/base-list-root.tsx (5 hunks)
- web/core/components/issues/issue-layouts/list/default.tsx (5 hunks)
- web/core/components/issues/issue-layouts/list/headers/group-by-card.tsx (3 hunks)
- web/core/components/issues/issue-layouts/list/list-group.tsx (5 hunks)
🧰 Additional context used
🪛 GitHub Check: lint-web
web/core/components/issues/issue-layouts/list/base-list-root.tsx
[failure] 19-19:
next/navigationimport should occur before import of@plane/types
[failure] 125-125:
Trailing spaces not allowedweb/core/components/issues/issue-layouts/list/default.tsx
[failure] 136-136:
Trailing spaces not allowed
🔇 Additional comments (29)
web/core/components/issues/issue-layouts/kanban/headers/sub-group-by-card.tsx (1)
17-17: LGTM! Consistent prop renaming.The renaming of
kanbanFilterstocollapsedGroupsin the props destructuring is consistent with the interface update. The order of destructured props is maintained, which is good for code readability.web/core/components/issues/issue-layouts/list/default.tsx (2)
17-17: LGTM: New import added correctly.The addition of
TIssueKanbanFiltersto the import statement is consistent with the changes made to theIListinterface.
73-74: LGTM: New props correctly destructured.The addition of
handleCollapsedGroupsandcollapsedGroupsto the props destructuring is consistent with the changes made to theIListinterface.web/core/components/issues/issue-layouts/list/headers/group-by-card.tsx (3)
32-32: LGTM: Interface update improves clarity and aligns with PR objectives.The replacement of
toggleListGroupwithhandleCollapsedGroupsin theIHeaderGroupByCardinterface provides more context about the method's functionality. This change aligns well with the PR's objective of implementing grouping persistence for the List View.
46-46: LGTM: Component changes correctly implement the interface updates.The
HeaderGroupByCardcomponent has been updated to use the newhandleCollapsedGroupsmethod from the props. TheonClickhandler now provides more specific information about the action being performed, which improves code readability and maintainability.Also applies to: 111-111
Line range hint
1-168: Summary: Changes successfully implement grouping persistence for List View.The modifications to the
HeaderGroupByCardcomponent and its interface successfully implement the grouping persistence functionality for the List View. The changes align well with the PR objectives and improve the overall user experience by maintaining the state of groupings across page refreshes.Key improvements:
- Updated interface with more specific method names.
- Correct implementation of the new
handleCollapsedGroupsmethod in the component.- Improved clarity and context in the onClick handler.
While the current implementation is sound, there's potential for future refactoring to simplify state management and improve overall component architecture.
web/core/components/issues/issue-layouts/kanban/headers/group-by-card.tsx (3)
27-28: Improved type safety and clarity in interface definition.The changes to the
IHeaderGroupByCardinterface enhance type safety and provide better clarity:
- Replacing
kanbanFilterswithcollapsedGroups: TIssueKanbanFiltersmore accurately describes the prop's purpose.- The new
handleCollapsedGroupsfunction signature is more specific, clearly defining the expected parameters and return type.These improvements will help prevent potential bugs and make the component's API more self-documenting.
Line range hint
1-186: Summary: Positive changes with considerations for broader impactThe changes to
HeaderGroupByCardrepresent a positive step towards improved type safety, clarity, and potentially more consistent state management across views. The refactoring fromkanbanFilterstocollapsedGroupsand the corresponding handler updates are well-implemented within this component.Recommendations:
- Thoroughly test the Kanban view to ensure grouping and collapsing functionality works as expected with these changes.
- Review and update all components that interact with
HeaderGroupByCardto ensure they provide the correct props.- Verify that state management for collapsed groups is consistent between List and Kanban views.
- Consider adding inline documentation to explain the purpose and usage of
collapsedGroups.- Update any relevant test cases to reflect these changes.
Overall, these changes appear to enhance the component's design, but careful integration testing is advised to ensure they don't introduce regressions in the broader application context.
41-42: Implementation aligned with interface changes.The component implementation has been correctly updated to use the new
collapsedGroupsandhandleCollapsedGroupsprops:
- Props destructuring updated (lines 41-42).
verticalAlignPositioncalculation now usescollapsedGroups(line 47).- Click handler updated to use
handleCollapsedGroups(line 136).These changes maintain consistency with the interface updates.
To ensure the changes don't introduce unexpected behavior, please verify:
- The grouping and collapsing functionality works as expected in the Kanban view.
- The vertical alignment of cards is correctly maintained based on the
collapsedGroupsstate.You can use the following script to check for any other occurrences of the old prop names that might have been missed:
Also applies to: 47-47, 136-136
web/core/components/issues/issue-layouts/kanban/default.tsx (3)
69-70: LGTM! Props destructuring is correct.The addition of
collapsedGroupsandhandleCollapsedGroupsto the props destructuring is consistent with the interface updates. This ensures that these new properties are available for use within theKanBancomponent.
Line range hint
1-200: Overall, the changes look good and align with the PR objectives.The implementation of
collapsedGroupsandhandleCollapsedGroupsis consistent throughout the file and aligns well with the PR objectives of implementing state persistence for groupings. The changes provide a more specific and type-safe way to handle collapsed groups in the Kanban view.A few minor suggestions have been made to improve readability and clarity, but these don't affect the overall functionality. Great job on implementing this feature!
164-165: LGTM! VerifyHeaderGroupByCardimplementation.The addition of
collapsedGroupsandhandleCollapsedGroupsas props to theHeaderGroupByCardcomponent is correct and consistent with the earlier changes. This allows the component to access and modify the collapsed state of groups.To ensure that these new props are correctly utilized, please verify the implementation of the
HeaderGroupByCardcomponent. Run the following script to check its implementation:✅ Verification successful
Action Required: Further Verification of
HeaderGroupByCardImplementationThe initial script failed to recognize the 'typescript' file type. Please use the following script to search within
.tsxfiles for theHeaderGroupByCardcomponent and its associated props:
Verified:
HeaderGroupByCardcorrectly utilizes the new props.The implementation of the
HeaderGroupByCardcomponent correctly handles thecollapsedGroupsandhandleCollapsedGroupsprops as introduced. All usages in the kanban-related files align with the updated interface, ensuring consistent behavior.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of HeaderGroupByCard component # Test: Search for the HeaderGroupByCard component definition and its usage of collapsedGroups and handleCollapsedGroups rg --type typescript -A 10 'const HeaderGroupByCard'Length of output: 89
Script:
# !/bin/bash # Description: Verify the implementation of HeaderGroupByCard component within .tsx files # Find all .tsx files and search for HeaderGroupByCard component usage fd --extension tsx | xargs rg 'HeaderGroupByCard' -A 10Length of output: 21917
web/core/components/issues/issue-layouts/kanban/base-kanban-root.tsx (2)
226-226: LGTM! Proper initialization of collapsedGroups.The assignment of
collapsedGroupscorrectly retrieves the collapsed groups state from the filters, with a sensible default value. This implementation supports the PR objective of persisting the state of groupings.
Line range hint
1-290: Overall, excellent implementation of group state persistence.The changes in this file effectively implement the functionality of persisting the "state of groupings" for the Kanban View, aligning well with the PR objectives. The code is well-structured, follows React best practices, and makes appropriate use of hooks for performance optimization.
A few minor suggestions have been made for improved clarity and consistency, but these are not critical issues. Great job on this implementation!
web/core/components/issues/issue-layouts/list/list-group.tsx (4)
16-16: LGTM: New props for managing collapsed groups.The addition of
handleCollapsedGroupsandcollapsedGroupsto the Props interface aligns well with the PR objectives. These props will enable the component to manage and persist the state of collapsed groups effectively.Also applies to: 63-64
89-90: LGTM: Proper destructuring of new props.The new props
handleCollapsedGroupsandcollapsedGroupsare correctly destructured from the props object, maintaining consistency with the Props interface changes.
249-249: LGTM: Passing handleCollapsedGroups to HeaderGroupByCard.The
handleCollapsedGroupsprop is correctly passed to the HeaderGroupByCard component, enabling it to manage the collapsed state of groups. This change is consistent with the PR objectives and the overall modifications in the file.
Line range hint
1-296: Overall assessment: Well-implemented persistence for groupings state.The changes in this file effectively implement the persistence of groupings state for the List View, aligning well with the PR objectives. The new props and logic for managing collapsed groups are appropriately integrated into the existing component structure. The code quality is good, with only minor suggestions for improvement in readability and consistency.
Key improvements:
- Addition of
handleCollapsedGroupsandcollapsedGroupsprops.- Derivation of
isExpandedstate fromcollapsedGroups.- Automatic expansion of groups on item drop.
- Proper propagation of new props to child components.
These changes should significantly enhance the user experience by maintaining the state of groupings across page refreshes and providing consistency between List and Kanban views.
web/core/components/issues/issue-layouts/kanban/swimlanes.tsx (9)
38-39: LGTM: Consistent renaming in ISubGroupSwimlaneHeader interfaceThe renaming of
kanbanFilterstocollapsedGroupsandhandleKanbanFilterstohandleCollapsedGroupsis consistent with the PR objectives. The types remain unchanged, maintaining the existing functionality while improving clarity.
56-56: LGTM: Consistent prop renaming in SubGroupSwimlaneHeader componentThe prop names have been updated consistently in the component's parameter destructuring and when passing them to the HeaderGroupByCard component. This change aligns with the interface updates and maintains the existing functionality.
Also applies to: 76-77
99-100: LGTM: Consistent renaming in ISubGroupSwimlane interfaceThe renaming of
kanbanFilterstocollapsedGroupsandhandleKanbanFilterstohandleCollapsedGroupsin the ISubGroupSwimlane interface is consistent with previous changes. The types remain unchanged, ensuring that the existing functionality is preserved.
123-124: LGTM: Consistent prop renaming and usage in SubGroupSwimlane componentThe prop names have been updated consistently in the component's parameter destructuring. The usage of
collapsedGroupsin the visibilitySubGroupBy function has been correctly updated. These changes align with the interface updates and maintain the existing functionality.Also applies to: 150-150
171-172: LGTM: Consistent prop renaming in HeaderSubGroupByCard usageThe prop names passed to the HeaderSubGroupByCard component have been updated consistently. This change aligns with previous updates and maintains the existing functionality.
189-190: LGTM: Consistent prop renaming in KanBan component usageThe prop names passed to the KanBan component have been updated consistently. This change aligns with previous updates and maintains the existing functionality.
226-227: LGTM: Consistent renaming in IKanBanSwimLanes interfaceThe renaming of
kanbanFilterstocollapsedGroupsandhandleKanbanFilterstohandleCollapsedGroupsin the IKanBanSwimLanes interface is consistent with previous changes. The types remain unchanged, ensuring that the existing functionality is preserved.
251-252: LGTM: Consistent prop renaming in KanBanSwimLanes componentThe prop names have been updated consistently in the component's parameter destructuring and when passing them to the SubGroupSwimlaneHeader and SubGroupSwimlane components. These changes align with the interface updates and previous changes, maintaining the existing functionality.
Also applies to: 305-306, 324-325
Line range hint
1-341: Summary: Consistent renaming improves code clarityThe changes in this file consistently rename
kanbanFilterstocollapsedGroupsandhandleKanbanFilterstohandleCollapsedGroupsacross all relevant interfaces and components. This renaming aligns with the PR objectives and improves code clarity by using more descriptive names for the grouping functionality. The existing functionality is maintained throughout these changes.Key points:
- All interfaces (ISubGroupSwimlaneHeader, ISubGroupSwimlane, IKanBanSwimLanes) have been updated.
- Components (SubGroupSwimlaneHeader, SubGroupSwimlane, KanBanSwimLanes) have been updated to use the new prop names.
- Usage of these props in child components (HeaderGroupByCard, HeaderSubGroupByCard, KanBan) has been updated.
- The visibilitySubGroupBy function has been updated to use the new
collapsedGroupsname.These changes contribute to a more consistent and understandable codebase, making it easier for developers to work with the Kanban swimlanes functionality.
web/core/components/issues/issue-layouts/list/base-list-root.tsx (2)
115-131:handleCollapsedGroupsfunction is well-implementedThe
handleCollapsedGroupsfunction correctly usesuseCallbackwith the necessary dependencies. It effectively manages the collapsed groups state shared between Kanban View and List View.🧰 Tools
🪛 GitHub Check: lint-web
[failure] 125-125:
Trailing spaces not allowed
154-155: Props passed correctly to theListcomponentThe
handleCollapsedGroupsandcollapsedGroupsprops are properly passed to the<List>component, ensuring consistent state persistence between views.
| onClick={() => handleCollapsedGroups("sub_group_by", column_id)} | ||
| > | ||
| <div className="flex h-[20px] w-[20px] flex-shrink-0 items-center justify-center overflow-hidden rounded-sm transition-all hover:bg-custom-background-80"> | ||
| {kanbanFilters?.sub_group_by.includes(column_id) ? ( | ||
| {collapsedGroups?.sub_group_by.includes(column_id) ? ( |
There was a problem hiding this comment.
💡 Codebase verification
Residual kanbanFilters Found
There are remaining instances of kanbanFilters in the following files, indicating that the renaming to collapsedGroups may be incomplete:
web/core/components/issues/issue-layouts/list/base-list-root.tsxweb/core/components/issues/issue-layouts/kanban/base-kanban-root.tsx
Please update these instances to ensure consistency across the codebase.
🔗 Analysis chain
LGTM! Verify the impact on related components.
The changes in the onClick handler and the condition check are consistent with the interface updates and prop renaming. The functionality appears to remain the same, with only the naming conventions updated.
To ensure that these changes don't introduce any unintended side effects, please run the following verification script:
This script will help identify any places where the old naming conventions might still be in use, and confirm that the new collapsedGroups terminology is used consistently across related components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of 'kanbanFilters' or 'handleKanbanFilters'
# in related components, and verify that 'collapsedGroups' is used consistently.
echo "Checking for remaining instances of old naming conventions:"
rg --type typescript 'kanbanFilters|handleKanbanFilters' web/core/components/issues/
echo "\nVerifying consistent usage of 'collapsedGroups':"
rg --type typescript 'collapsedGroups' web/core/components/issues/
Length of output: 469
Script:
#!/bin/bash
# Description: Check for any remaining instances of 'kanbanFilters' or 'handleKanbanFilters'
# in related components, and verify that 'collapsedGroups' is used consistently.
echo "Checking for remaining instances of old naming conventions:"
rg --type ts 'kanbanFilters|handleKanbanFilters' web/core/components/issues/
echo "\nVerifying consistent usage of 'collapsedGroups':"
rg --type ts 'collapsedGroups' web/core/components/issues/
Length of output: 6365
| } else { | ||
| collapsedGroups.push(value); | ||
| } | ||
| updateFilters(projectId?.toString() ?? "", EIssueFilterType.KANBAN_FILTERS, |
There was a problem hiding this comment.
Remove trailing spaces to satisfy linting rules
There are trailing spaces at the end of line 125, which violate the linting rules.
Apply this diff to remove the trailing spaces:
- updateFilters(projectId?.toString() ?? "", EIssueFilterType.KANBAN_FILTERS,
+ updateFilters(projectId?.toString() ?? "", EIssueFilterType.KANBAN_FILTERS,📝 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.
| updateFilters(projectId?.toString() ?? "", EIssueFilterType.KANBAN_FILTERS, | |
| updateFilters(projectId?.toString() ?? "", EIssueFilterType.KANBAN_FILTERS, |
🧰 Tools
🪛 GitHub Check: lint-web
[failure] 125-125:
Trailing spaces not allowed
| @@ -16,6 +16,7 @@ | |||
| import { List } from "./default"; | |||
| // types | |||
| import { IQuickActionProps, TRenderQuickActions } from "./list-view-types"; | |||
| import { useParams } from "next/navigation"; | |||
There was a problem hiding this comment.
Fix import order to comply with linting rules
According to the linting rules, the import from "next/navigation" should appear before the import from "@plane/types".
Apply this diff to correct the import order:
+import { useParams } from "next/navigation";
import { GroupByColumnTypes, TGroupedIssues, TIssueKanbanFilters } from "@plane/types";Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 GitHub Check: lint-web
[failure] 19-19:
next/navigationimport should occur before import of@plane/types
There was a problem hiding this comment.
@sharma01ketan One minor change, and also please fix lint errors, looks like they are failing on the PR
| collapsedGroups.push(value); | ||
| // kanbanFilters and EIssueFilterType.KANBAN_FILTERS are used becuase the state is shared between kanban view and list view | ||
| const handleCollapsedGroups = useCallback( | ||
| (toggle: "group_by", value: string) => { |
There was a problem hiding this comment.
you can also remove toggle argument completely since it is always "group_by" for list
There was a problem hiding this comment.
Done, made the changes.
…/list-kanban-grouping
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
web/core/components/issues/issue-layouts/list/base-list-root.tsx (2)
63-65: LGTM: Hooks and state initialization look good.The use of
useParamsfor extracting URL parameters and the initialization ofcollapsedGroupsare implemented correctly. TheupdateFiltersfunction is properly obtained from theuseIssuesActionshook.Consider using a default empty object for
kanbanFiltersto simplify the initialization:const collapsedGroups = issuesFilter?.issueFilters?.kanbanFilters ?? { group_by: [], sub_group_by: [] } as TIssueKanbanFilters;This change would make the code slightly more concise and easier to read.
115-131: LGTM: ThehandleCollapsedGroupsfunction is well-implemented.The function correctly toggles values in the
collapsedGroupsarray and updates the filters using theupdateFiltersmethod. The use ofuseCallbackis appropriate for performance optimization.Consider using the spread operator for a more concise array update:
const updatedGroups = collapsedGroups.includes(value) ? collapsedGroups.filter((_value) => _value !== value) : [...collapsedGroups, value]; updateFilters(projectId?.toString() ?? "", EIssueFilterType.KANBAN_FILTERS, { group_by: updatedGroups } as TIssueKanbanFilters );This change would make the code slightly more readable and reduce the number of lines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- web/core/components/issues/issue-layouts/kanban/base-kanban-root.tsx (2 hunks)
- web/core/components/issues/issue-layouts/kanban/default.tsx (4 hunks)
- web/core/components/issues/issue-layouts/kanban/headers/group-by-card.tsx (3 hunks)
- web/core/components/issues/issue-layouts/kanban/swimlanes.tsx (12 hunks)
- web/core/components/issues/issue-layouts/list/base-list-root.tsx (4 hunks)
- web/core/components/issues/issue-layouts/list/default.tsx (4 hunks)
- web/core/components/issues/issue-layouts/list/headers/group-by-card.tsx (3 hunks)
- web/core/components/issues/issue-layouts/list/list-group.tsx (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- web/core/components/issues/issue-layouts/kanban/base-kanban-root.tsx
- web/core/components/issues/issue-layouts/kanban/default.tsx
- web/core/components/issues/issue-layouts/kanban/headers/group-by-card.tsx
- web/core/components/issues/issue-layouts/kanban/swimlanes.tsx
- web/core/components/issues/issue-layouts/list/default.tsx
- web/core/components/issues/issue-layouts/list/headers/group-by-card.tsx
- web/core/components/issues/issue-layouts/list/list-group.tsx
🔇 Additional comments (3)
web/core/components/issues/issue-layouts/list/base-list-root.tsx (3)
4-7: LGTM: Import changes are appropriate and correctly ordered.The new imports for
useParams,TIssueKanbanFilters, andEIssueFilterTypeare necessary for the added functionality. The import order satisfies the linting rules as mentioned in the past review comments.
154-155: LGTM: New props are correctly passed to theListcomponent.The addition of
handleCollapsedGroupsandcollapsedGroupsprops to theListcomponent is correct. This change enables theListcomponent to manage and reflect changes in issue grouping based on user actions, improving the component's functionality.
Line range hint
1-160: Overall changes align well with PR objectives.The implementation successfully addresses the main objectives of the PR:
- It implements persistence of the "state of groupings" for the List View.
- It introduces a shared state mechanism between the List View and the Kanban View.
- It corrects previously set values for the keys.
The use of
EIssueFilterType.KANBAN_FILTERSand theupdateFiltersfunction ensures that the state is shared and persisted correctly. These changes enhance the functionality and user experience of the application by maintaining consistent grouping states across views and page refreshes.
[WEB-2213]
Changes:
New State:
Screen.Recording.2024-09-12.at.2.40.10.PM.mov
Previous State:
Screen.Recording.2024-09-12.at.2.44.18.PM.mov
[WEB-2213]
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements
Documentation