Skip to content
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

Tooltip support for TableView actions #5142

Merged
merged 3 commits into from
Nov 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 30 additions & 15 deletions app/packages/core/src/plugins/SchemaIO/components/ActionsMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
Menu,
MenuItem,
Stack,
Tooltip,
} from "@mui/material";
import React, { useCallback } from "react";
import { getColorByCode } from "../utils";
Expand All @@ -20,7 +21,7 @@ export default function ActionsMenu(props: ActionsPropsType) {

if (actions.length <= maxInline) {
return (
<Stack direction="row" spacing={0.5} justifyContent="flex-end">
<Stack direction="row" spacing={0.5} justifyContent="flex-start">
{actions.map((action) => (
<Action {...action} key={action.name} mode="inline" size={size} />
))}
Expand Down Expand Up @@ -73,7 +74,8 @@ function ActionsOverflowMenu(props: ActionsPropsType) {
}

function Action(props: ActionPropsType) {
const { label, name, onClick, icon, variant, mode, color, size } = props;
const { label, name, onClick, icon, variant, mode, color, size, tooltip } =
props;
const resolvedColor = color ? getColorByCode(color) : undefined;

const Icon = icon ? (
Expand All @@ -87,20 +89,32 @@ function Action(props: ActionPropsType) {
[onClick, props]
);

return mode === "inline" ? (
<Button
variant={variant}
startIcon={Icon}
onClick={handleClick}
sx={{ color: resolvedColor, padding: size === "small" ? 0 : undefined }}
>
{label}
</Button>
const content =
mode === "inline" ? (
<Button
variant={variant}
startIcon={Icon}
onClick={handleClick}
sx={{ color: resolvedColor, padding: size === "small" ? 0 : undefined }}
>
{label}
</Button>
) : (
<MenuItem onClick={handleClick}>
{Icon && <ListItemIcon>{Icon}</ListItemIcon>}
<ListItemText sx={{ color: resolvedColor }}>
{label || name}
</ListItemText>
</MenuItem>
);

return tooltip ? (
<Tooltip title={tooltip}>
<span>{content}</span>{" "}
{/* Use <span> to wrap the child to avoid DOM issues */}
</Tooltip>
) : (
<MenuItem onClick={handleClick}>
{Icon && <ListItemIcon>{Icon}</ListItemIcon>}
<ListItemText sx={{ color: resolvedColor }}>{label || name}</ListItemText>
</MenuItem>
content
);
}

Expand All @@ -121,4 +135,5 @@ type ActionPropsType = {
mode: "inline" | "menu";
color?: string;
size?: SizeType;
tooltip: string | null;
};
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,9 @@ export default function TableView(props: ViewPropsType) {
{hasRowActions && (
<TableCell
{...getComponentProps(props, "tableHeadCell", {
sx: { ...headingCellBaseStyles, textAlign: "right" },
sx: { ...headingCellBaseStyles, textAlign: "left" },
})}
width={`${max_inline_actions * 5}%`}
Comment on lines +144 to +146
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Fix inconsistent alignment and improve width calculation

There are two issues with the actions column:

  1. The header is left-aligned but the content in table cells (line 198) is right-aligned, creating an inconsistent visual appearance.
  2. The width calculation (max_inline_actions * 5%) might be too narrow for action buttons with text, especially with the default max_inline_actions=1.

Consider these improvements:

 <TableCell
   {...getComponentProps(props, "tableHeadCell", {
-    sx: { ...headingCellBaseStyles, textAlign: "left" },
+    sx: { ...headingCellBaseStyles, textAlign: "right" },
   })}
-  width={`${max_inline_actions * 5}%`}
+  width={`${Math.max(max_inline_actions * 8, 10)}%`}
   onClick={() => {
     handleCellClick(-1, -1);
   }}
 >

This change:

  • Aligns the header with the content
  • Ensures a minimum width of 10% and allocates 8% per action instead of 5%
📝 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.

Suggested change
sx: { ...headingCellBaseStyles, textAlign: "left" },
})}
width={`${max_inline_actions * 5}%`}
sx: { ...headingCellBaseStyles, textAlign: "right" },
})}
width={`${Math.max(max_inline_actions * 8, 10)}%`}

onClick={() => {
handleCellClick(-1, -1);
}}
Expand Down
5 changes: 3 additions & 2 deletions fiftyone/operators/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -1816,6 +1816,7 @@ class Action(View):
name: the name of the action
label (None): the label of the action
icon (None): the icon of the action
tooltip (None): the tooltip of the action
on_click: the operator to execute when the action is clicked
"""

Expand Down Expand Up @@ -1851,9 +1852,9 @@ def add_column(self, key, **kwargs):
self.columns.append(column)
return column

def add_row_action(self, name, on_click, label=None, icon=None, **kwargs):
def add_row_action(self, name, on_click, label=None, icon=None, tooltip=None, **kwargs):
row_action = Action(
name=name, on_click=on_click, label=label, icon=icon, **kwargs
name=name, on_click=on_click, label=label, icon=icon, tooltip=tooltip, **kwargs
)
self.row_actions.append(row_action)
return row_action
Expand Down
Loading