NEO-2435: Global Custom Filter improvements #560
Conversation
WalkthroughThe changes in this pull request enhance the Changes
Possibly related PRs
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 (
|
✅ Deploy Preview for neo-react-library-storybook ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
src/components/Table/types/TableTypes.ts (1)
Line range hint
1-106: Consider improving overall type safety and documentationWhile the file structure is generally good, there are a few areas where it could be improved:
Type safety: The use of
anyinAnyRecordand other places reduces type safety. Consider using more specific types where possible.Documentation: Many types and interfaces lack JSDoc comments. Adding these would improve code understanding and maintainability.
Consistency: The file mixes interfaces and type aliases. While not incorrect, choosing one style could improve consistency.
Here are some suggestions:
Replace
AnyRecordwith a more specific type when possible, or use generics to constrain the type.Add JSDoc comments to major types and interfaces. For example:
/** * Represents the properties for the Table component. * @template T The type of data in the table rows. */ export type TableProps<T extends AnyRecord> = { // ... existing properties }Consider using either interfaces or type aliases consistently throughout the file, unless there's a specific reason to mix them.
These changes would make the code more robust, self-documenting, and consistent.
src/components/Table/Table.tsx (1)
209-214: Approve new useEffect for filter updates, but consider refinementsThe new useEffect hook appropriately updates the table's filters when
allFilterschanges. However, consider the following suggestions:
- Remove the
logger.infostatement for production builds to avoid unnecessary console output.- Reconsider the dependency array. While the lint rule is disabled, it's generally better to include all dependencies. If
setAllFiltersis stable (e.g., from useCallback), including it won't cause issues.Consider applying these changes:
useEffect(() => { - logger.info(allFilters, filters); setAllFilters(allFilters); -}, [allFilters]); +}, [allFilters, setAllFilters]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/components/Table/Table.stories.tsx (1 hunks)
- src/components/Table/Table.tsx (2 hunks)
- src/components/Table/types/TableTypes.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
src/components/Table/Table.tsx (4)
117-117: LGTM: NewallFiltersprop enhances filtering capabilitiesThe addition of the
allFiltersprop with a default empty array is a good enhancement. It allows for more flexible and dynamic filter application while maintaining backward compatibility.
200-200: LGTM: Destructuredfiltersfrom state enhances filter managementThe addition of
filtersto the destructured state fromuseTablehook is a good practice. It allows for better management and access to the current filters applied to the table, supporting the enhanced filtering functionality.
206-206: LGTM: AddedsetAllFiltersfor comprehensive filter updatesThe inclusion of
setAllFiltersfrom theuseTablehook is appropriate. This function will allow for updating all filters simultaneously, which is consistent with the newallFiltersprop and enhances the table's filtering capabilities.
Line range hint
269-275: LGTM: ImprovedhandleDeleteWrapperwith row deselectionThe addition of
toggleAllRowsSelected(false)after the delete operation is a good improvement. It ensures that all rows are deselected after a delete operation, preventing potential confusion or unintended actions on deleted rows.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
src/components/Table/Table.stories.tsx (1)
Line range hint
1097-1170: DisabledRows story demonstrates disabled rows functionalityThis story effectively showcases the Table component with disabled rows. It implements:
- State management for data and readonly status.
- Handlers for create, delete, edit, and refresh actions.
- Logging of data modifications.
The implementation is consistent with other stories and demonstrates good practices.
Consider adding a comment or visual indicator in the story to explicitly show how rows are disabled in the data. This would make it clearer for developers how to implement disabled rows in their own tables.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/components/Table/Table.stories.tsx (2 hunks)
- src/components/Table/Table.tsx (2 hunks)
- src/components/Table/types/TableTypes.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Table/types/TableTypes.ts
🧰 Additional context used
🔇 Additional comments (11)
src/components/Table/Table.stories.tsx (7)
44-44: New import added for ColumnFilter typeThe
ColumnFiltertype has been imported from "./types". This suggests that filter-related functionality has been added or modified in the Table component.
893-894: Update Table component with allFilters propThe Table component is now using a new prop 'allFilters' to apply filters. This is a good addition for managing filters externally.
903-915: Add 'value' prop to TextInput componentsThe TextInput components for name and other filters are missing the 'value' prop, making them uncontrolled components. This issue was identified in a previous review and is still valid.
To fix this, add the 'value' prop to both TextInput components:
<TextInput clearable label="Name" type="text" + value={nameFilter ?? ''} onChange={(e) => setNameFilter(e.currentTarget.value)} /> <TextInput clearable label="Other" type="text" + value={otherFilter ?? ''} onChange={(e) => setOtherFilter(e.currentTarget.value)} />
Line range hint
919-923: WithRowSeparator story looks goodThis story effectively demonstrates the usage of the 'showRowSeparator' prop for the Table component. The implementation is clean and straightforward.
Line range hint
925-979: PreSelectedRows story demonstrates good practicesThis story effectively showcases the pre-selection of rows in the Table component. It demonstrates several good practices:
- Uses the 'defaultSelectedRowIds' prop to set initial selected rows.
- Implements a custom 'handleToggle' function to manage row selection state.
- Provides visual feedback through logging of selection changes.
- Uses the 'selectableRows' prop to enable multiple row selection.
The implementation is clean, well-structured, and serves as a good example for managing row selection in the Table component.
Line range hint
981-1095: SecondPage story provides comprehensive pagination exampleThis story excellently demonstrates advanced pagination functionality:
- Defaults to the second page and tracks the current page index.
- Manages state for data, readonly status, page index, and page size.
- Implements handlers for create, delete, edit, and refresh actions.
- Provides clear explanations of the functionality through comments and UI text.
- Demonstrates how to handle page changes and maintain the current page when data is modified.
The implementation is thorough and serves as a great example for managing complex pagination scenarios in the Table component.
Line range hint
1172-1295: EmbeddedSelects story demonstrates advanced table functionalityThis story effectively showcases embedding Select components within table cells and implementing custom action handling for each row. The implementation is creative and demonstrates advanced usage of the Table component.
Consider the following improvements:
- Memoize the handleSelectedValueChange function using useCallback to optimize performance.
- Simplify the switch statement in handleSelectedValueChange by using an object lookup or reducing repetitive code.
There's a HACK comment mentioning that actions are being called twice. This needs investigation:
This will help identify if the issue is in the Table component itself or if it's specific to this story's implementation.
✅ Verification successful
Verified: No additional instances of double action calls found beyond the scoped component. The HACK in
Table.stories.tsxeffectively addresses the issue within this context.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential double action calls in the Table component rg --type typescript 'handleSelectedValueChange|onChange.*Select' src/components/TableLength of output: 123
Script:
#!/bin/bash # Search for potential double action calls in the Table component with correct file extensions rg 'handleSelectedValueChange|onChange.*Select' src/components/Table --glob '*.ts' --glob '*.tsx'Length of output: 270
src/components/Table/Table.tsx (4)
117-117: Addition ofallFilterspropThe
allFiltersprop is correctly added to the component's props with a default value of an empty array. This allows the table to accept multiple filters externally.
200-200: Destructuringfiltersfrom stateDestructuring
filtersfrom the state enables access to the current filters applied to the table, which is necessary for managing filter states effectively.
206-206: DestructuringsetAllFiltersfrom the table instanceExtracting
setAllFiltersfrom the table instance allows for programmatically updating all filters, which is essential for the new filtering functionality.
209-215: Consider updating the dependency array inuseEffectThe
useEffecthook does not includesetAllFiltersin its dependency array and includes a comment to ignore exhaustive dependencies for performance reasons. While this might prevent unnecessary re-renders, omitting dependencies can lead to bugs ifsetAllFilterschanges. Ensure thatsetAllFiltersis stable or consider including it in the dependency array.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/components/Table/Table.tsx (2 hunks)
🧰 Additional context used
🔇 Additional comments (5)
src/components/Table/Table.tsx (5)
117-117: LGTM: NewallFiltersprop addedThe addition of the
allFiltersprop with a default empty array is a good enhancement. It allows for external control of filters while maintaining backward compatibility.
200-200: LGTM: DestructuringfiltersfromuseTablestateExtracting
filtersfrom theuseTablehook's state is a good practice. It allows for easier access to the current filters applied to the table, which is essential for the new filtering functionality.
206-206: LGTM: DestructuringsetAllFiltersfromuseTableThe addition of
setAllFiltersto the destructured values fromuseTableis appropriate. This function will be crucial for updating all filters simultaneously, which aligns with the new filtering functionality introduced by theallFiltersprop.
Line range hint
284-291: LGTM: ImprovedhandleDeleteWrapperfunctionThe modification to
handleDeleteWrapperis a good improvement. CallingtoggleAllRowsSelected(false)after the delete operation ensures that all rows are deselected, which is a good practice after performing a bulk action like delete. This prevents potential confusion or unintended actions on stale selections.
Line range hint
1-624: Overall assessment: Good improvements with one concernThe changes to the Table component generally enhance its functionality, particularly in terms of filtering and state management. The addition of the
allFiltersprop and related logic improves the component's flexibility. However, there's one important issue to address:
- The suppressed TypeScript error in the new useEffect hook should be resolved rather than ignored.
Once this issue is addressed, the changes will significantly improve the Table component without introducing potential type-related bugs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/components/Table/index.ts (2)
2-2: LGTM! Consider adding a brief comment for the new export.The addition of
ColumnFilterto the exports aligns well with the PR objective of enhancing global custom filter functionality. This change will allow consumers of the library to access and use theColumnFiltertype, which is likely crucial for implementing the new filtering capabilities.Consider adding a brief comment above the export line to explain the purpose of
ColumnFilter. This can help other developers understand its role in the new filtering system. For example:// ColumnFilter type used for configuring custom column filters export type { ColumnFilter, TableProps } from "./types";
Line range hint
6-6: Consider enhancing the note aboutTableNextThe note about
TableNextnot being exported is informative. To provide more context for other developers, consider expanding this note slightly.You could modify the note to include more details or a timeline if available:
// NOTE: `TableNext` is *not* exported as it is not ready for production use. // It is under active development and is expected to be available in a future release. // For updates, please check the project roadmap or upcoming release notes.This additional information can help set expectations for when
TableNextmight become available and where to look for updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- package.json (1 hunks)
- src/components/Table/index.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🧰 Additional context used
📓 Learnings (1)
📓 Common learnings
Learnt from: enrique-prado PR: avaya-dux/neo-react-library#560 File: src/components/Table/Table.stories.tsx:866-885 Timestamp: 2024-10-10T17:30:21.136Z Learning: In `src/components/Table/Table.stories.tsx`, suggestions to `CustomBasicTableFilterDrawer` should focus on significant and relevant improvements, as minor suggestions may be considered irrelevant.
Link to updated story in Deploy Preview
Before tagging the team for a review, I have done the following:
yarn alllocally: ensures that all tests pass, formatting is done, types pass, and builds passaxe Dev Tools, Mac's VoiceOver, etc.)@avaya-dux/dux-devsThis PR adds the ability to Apply multiple column filters while providing a custom UI for specifying those filters.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Functionality Enhancements
allFiltersprop for dynamic filter application.Bug Fixes
Documentation