Add support for cell tooltip in TableView #5152
Conversation
WalkthroughThe changes in this pull request enhance the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
Documentation and Community
|
| @@ -170,13 +182,14 @@ export default function TableView(props: ViewPropsType) { | |||
| selectedCells.has(coordinate) || | |||
| isRowSelected || | |||
| selectedColumns.has(columnIndex); | |||
| return ( | |||
|
|
|||
| const tooltip = tooltipMap[coordinate]; // Check if there's a tooltip for the cell | |||
There was a problem hiding this comment.
Here we don't check if coordinate is a valid key, the assumption is that tooltipMap will return null if the key is not in the map
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
app/packages/core/src/plugins/SchemaIO/components/TableView.tsx (2)
69-75: Improve tooltip mapping implementationThe
getTooltipsimplementation has several areas for improvement:
- Missing TypeScript types
- Unnecessary recreation on every render due to empty dependency array
- Could use object literal initialization for better performance
Consider this improved implementation:
const getTooltips = useCallback((tooltipList: TooltipDefinition[]) => { return tooltipList.reduce((acc, { value, row, column }) => { acc[`${row},${column}`] = value; return acc; }, {} as Record<string, string>); }, [/* no dependencies needed */]);
Line range hint
185-209: Simplify cell rendering logic and avoid coordinate string duplicationThe current implementation duplicates the coordinate string creation logic and has nested ternary expressions that could be simplified.
Consider this cleaner implementation:
const coordinate = `${rowIndex},${columnIndex}`; const tooltip = tooltipMap[coordinate]; const cell = ( <TableCell key={key} sx={{ background: isSelected ? selectedCellColor : "unset", }} onClick={() => handleCellClick(rowIndex, columnIndex)} {...getComponentProps(props, "tableBodyCell")} > {formatCellValue(item[key], props)} </TableCell> ); return tooltip ? ( <Tooltip key={key} title={tooltip} arrow> {cell} </Tooltip> ) : cell;This approach:
- Creates the coordinate string once
- Improves readability by reducing nesting
- Maintains the same functionality
fiftyone/operators/types.py (1)
1841-1858: Enhance Tooltip class documentation and implementation.The class documentation and implementation could be improved:
- Add usage examples in the docstring
- Define explicit attributes for
row,column, andvaluementioned in the docstringclass Tooltip(View): """A tooltip (currently supported only in a :class:`TableView`). Args: value: the value of the tooltip row: the row of the tooltip column: the column of the tooltip + + Examples:: + + import fiftyone.operators.types as types + + table_view = types.TableView() + table_view.add_tooltip( + row=0, + column="status", + value="Processing complete" + ) """ def __init__(self, **kwargs): super().__init__(**kwargs) + self.value = kwargs.get("value") + self.row = kwargs.get("row") + self.column = kwargs.get("column") def clone(self): clone = Tooltip(**self._kwargs) return clone def to_json(self): - return {**super().to_json()} + return { + **super().to_json(), + "value": self.value, + "row": self.row, + "column": self.column + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/packages/core/src/plugins/SchemaIO/components/TableView.tsx(6 hunks)fiftyone/operators/types.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/core/src/plugins/SchemaIO/components/TableView.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (2)
app/packages/core/src/plugins/SchemaIO/components/TableView.tsx (1)
107-107: Verify tooltip cleanup and performance
The tooltip map is recreated on every render. Let's verify there are no memory leaks or performance issues.
✅ Verification successful
Let me gather more information about the tooltip implementation and cleanup patterns.
Let me try one more search pattern to find the tooltip implementation.
No performance concerns with tooltip implementation
The implementation is well-optimized:
getTooltipsis memoized withuseCallback- Tooltips are stored in a simple object (not Map) with efficient key-value lookups
- The tooltip component is only rendered when a tooltip exists for a cell
- MUI's Tooltip component handles its own cleanup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other tooltip implementations in the codebase for consistency
rg -g '*.{ts,tsx}' 'Tooltip.*title=' --type-add 'tsx:*.{ts,tsx}'
# Look for cleanup patterns in similar components
ast-grep --pattern 'useEffect($$$) {
$$$
return () => {
$$$
}
}'
Length of output: 2065
Script:
#!/bin/bash
# Check the implementation of getTooltips function
ast-grep --pattern 'getTooltips = ($$$) => {
$$$
}'
# Look for tooltip cleanup or disposal patterns
rg -g '*.{ts,tsx}' 'useEffect.*tooltip' --type-add 'tsx:*.{ts,tsx}' -B 2 -A 5
# Check TableView component for any cleanup effects
rg -g '*TableView.tsx' 'useEffect' -B 2 -A 5
Length of output: 180
Script:
#!/bin/bash
# Search for getTooltips function with a more flexible pattern
rg -g '*.{ts,tsx}' 'function.*getTooltips|const.*getTooltips' --type-add 'tsx:*.{ts,tsx}' -B 2 -A 10
# Check TableView imports and tooltip-related code
rg -g '*TableView.tsx' 'tooltip|Tooltip' --type-add 'tsx:*.{ts,tsx}' -B 2 -A 2
# Look for any Map usage in TableView
rg -g '*TableView.tsx' 'new Map|Map<' --type-add 'tsx:*.{ts,tsx}' -B 2 -A 2
Length of output: 6740
fiftyone/operators/types.py (1)
1873-1873: LGTM! TableView tooltip integration is well implemented.
The changes to TableView properly integrate tooltip support:
- Initialization of tooltips list
- Method to add tooltips
- Proper handling in clone and to_json methods
Also applies to: 1889-1893, 1899-1899, 1907-1907
| @@ -34,11 +35,13 @@ export default function TableView(props: ViewPropsType) { | |||
| size = "small", | |||
| variant = "filled", | |||
| max_inline_actions = 1, | |||
| tooltips = [] | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add TypeScript type definition for tooltips prop
The tooltips prop is initialized with an empty array but lacks type information. This could lead to type-safety issues.
Consider adding an interface:
interface TooltipDefinition {
value: string;
row: number;
column: number;
}Then update the prop:
-tooltips = []
+tooltips: TooltipDefinition[] = []
app/packages/core/src/plugins/SchemaIO/components/TableView.tsx
Outdated
Show resolved
Hide resolved
|
Is there a way to make the tooltip on the content of the cell and not the cell itself? Looks a bit weird so off center from the actual text |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/packages/core/src/plugins/SchemaIO/components/TableView.tsx (1)
184-184: Remove unnecessary whitespaceThere's an extra blank line that can be removed to maintain consistent spacing.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/packages/core/src/plugins/SchemaIO/components/TableView.tsx(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/core/src/plugins/SchemaIO/components/TableView.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (4)
app/packages/core/src/plugins/SchemaIO/components/TableView.tsx (4)
13-13: Add type definition for tooltips prop
The existing review comment about adding TypeScript type definition for the tooltips prop is still valid. This would improve type safety and developer experience.
Also applies to: 38-38
68-74: LGTM! Well-implemented tooltip mapping function
The getTooltips implementation is clean, efficient, and properly memoized. The coordinate key format aligns well with the existing selectedCells implementation.
106-106: LGTM! Efficient tooltip map initialization
The tooltipMap is correctly initialized outside the render loop, preventing unnecessary recalculations.
Line range hint 187-208: LGTM! Clean tooltip rendering implementation
The cell rendering logic with tooltips is well-structured and maintainable. The conditional rendering approach using the ternary operator is appropriate here.
|
@CamronStaley: updated to wrap around cell content
|
What changes are proposed in this pull request?
Added support for cell tooltip in TableView
How is this patch tested? If it is not, please explain why.
(the current panel is a local version for testing, not the RC version, so a few details might be a bit off)
Example usage:
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
TableViewcomponent.