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

TableView enhancements #4809

Merged
merged 1 commit into from
Oct 21, 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
108 changes: 108 additions & 0 deletions app/packages/core/src/plugins/SchemaIO/components/ActionsMenu.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
import { MuiIconFont } from "@fiftyone/components";
import { MoreVert } from "@mui/icons-material";
import {
Box,
Button,
IconButton,
ListItemIcon,
ListItemText,
Menu,
MenuItem,
Stack,
} from "@mui/material";
import React, { useCallback } from "react";

const DEFAULT_MAX_INLINE = 1;

export default function ActionsMenu(props: ActionsPropsType) {
const { actions, maxInline = DEFAULT_MAX_INLINE } = props;

if (actions.length === maxInline) {
return (
<Stack direction="row" spacing={0.5} justifyContent="flex-end">
{actions.map((action) => (
<Action {...action} key={action.name} mode="inline" />
))}
</Stack>
);
}

return <ActionsOverflowMenu actions={actions} />;
}

function ActionsOverflowMenu(props: ActionsPropsType) {
const { actions } = props;
const [open, setOpen] = React.useState(false);
const anchorRef = React.useRef(null);

const handleClose = useCallback(() => {
setOpen(false);
}, []);

return (
<Box>
<IconButton
onClick={() => {
setOpen(!open);
}}
ref={anchorRef}
>
<MoreVert />
</IconButton>
<Menu open={open} onClose={handleClose} anchorEl={anchorRef.current}>
{actions.map((action) => {
const { name, onClick } = action;
return (
<Action
key={name}
{...action}
mode="menu"
onClick={(action, e) => {
handleClose();
onClick?.(action, e);
}}
/>
);
})}
</Menu>
</Box>
);
}

function Action(props: ActionPropsType) {
const { label, name, onClick, icon, variant, mode } = props;

const Icon = icon ? <MuiIconFont name={icon} /> : null;

const handleClick = useCallback(
(e: React.MouseEvent) => {
onClick?.(props, e);
},
[onClick, props]
);

return mode === "inline" ? (
<Button variant={variant} startIcon={Icon} onClick={handleClick}>
{label}
</Button>
) : (
<MenuItem onClick={handleClick}>
{Icon && <ListItemIcon>{Icon}</ListItemIcon>}
<ListItemText>{label || name}</ListItemText>
</MenuItem>
);
}

type ActionsPropsType = {
actions: Array<ActionPropsType>;
maxInline?: number;
};

type ActionPropsType = {
name: string;
label: string;
onClick: (action: ActionPropsType, e: React.MouseEvent) => void;
icon: string;
variant: string;
mode: "inline" | "menu";
};
Comment on lines +96 to +108
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

Improve type definitions for better type safety and flexibility.

The current type definitions are good, but can be enhanced:

  1. For ActionsPropsType, consider using a more specific type for maxInline:
type ActionsPropsType = {
  actions: Array<ActionPropsType>;
  maxInline?: 1 | 2 | 3 | 4 | 5; // or whatever reasonable maximum you want to allow
};
  1. For ActionPropsType, make some properties optional and use a union type for mode:
type BaseActionProps = {
  name: string;
  label: string;
  onClick: (action: ActionPropsType, e: React.MouseEvent) => void;
  icon?: string;
};

type InlineActionProps = BaseActionProps & {
  mode: "inline";
  variant: string;
};

type MenuActionProps = BaseActionProps & {
  mode: "menu";
};

type ActionPropsType = InlineActionProps | MenuActionProps;

These changes would improve type safety and make the types more accurately reflect how they're used in the components.

184 changes: 163 additions & 21 deletions app/packages/core/src/plugins/SchemaIO/components/TableView.tsx
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
import { scrollable } from "@fiftyone/components";
import { usePanelEvent } from "@fiftyone/operators";
import { usePanelId } from "@fiftyone/spaces";
import {
Box,
Paper,
Expand All @@ -8,23 +11,73 @@ import {
TableHead,
TableRow,
} from "@mui/material";
import React from "react";
import { isPlainObject } from "lodash";
import React, { useCallback } from "react";
import { HeaderView } from ".";
import EmptyState from "./EmptyState";
import { getComponentProps } from "../utils";
import { ViewPropsType } from "../utils/types";
import ActionsMenu from "./ActionsMenu";
import EmptyState from "./EmptyState";

export default function TableView(props) {
const { schema, data } = props;
const { view = {}, default: defaultValue } = schema;
const { columns } = view;
export default function TableView(props: ViewPropsType) {
const { path, schema } = props;
const { view = {} } = schema;
const {
columns,
row_actions = [],
on_click_cell,
on_click_row,
on_click_column,
actions_label,
} = view;
const { rows, selectedCells, selectedRows, selectedColumns } =
getTableData(props);
const dataMissing = rows.length === 0;
const hasRowActions = row_actions.length > 0;
const panelId = usePanelId();
const handleClick = usePanelEvent();

const table = Array.isArray(data)
? data
: Array.isArray(defaultValue)
? defaultValue
: [];
const getRowActions = useCallback((row) => {
const computedRowActions = [] as any;
for (const action of row_actions) {
if (action.rows?.[row] !== false) {
computedRowActions.push({
...action,
onClick: (action, e) => {
handleClick(panelId, {
operator: action.on_click,
params: { path, event: action.name, row },
});
},
});
}
}
return computedRowActions;
}, []);

const dataMissing = table.length === 0;
const handleCellClick = useCallback(
(row, column) => {
if (on_click_cell) {
handleClick(panelId, {
operator: on_click_cell,
params: { row, column, path, event: "on_click_cell" },
});
}
if (on_click_row) {
handleClick(panelId, {
operator: on_click_row,
params: { row, path, event: "on_click_row" },
});
}
if (on_click_column) {
handleClick(panelId, {
operator: on_click_column,
params: { column, path, event: "on_click_column" },
});
}
},
[on_click_cell, on_click_row, on_click_column, handleClick, panelId, path]
);

return (
<Box {...getComponentProps(props, "container")}>
Expand All @@ -33,37 +86,73 @@ export default function TableView(props) {
{!dataMissing && (
<TableContainer
component={Paper}
className={scrollable}
{...getComponentProps(props, "tableContainer")}
>
<Table sx={{ minWidth: 650 }} {...getComponentProps(props, "table")}>
<TableHead {...getComponentProps(props, "tableHead")}>
<TableRow {...getComponentProps(props, "tableHeadRow")}>
{columns.map(({ key, label }) => (
{columns.map(({ key, label }, columnIndex) => (
<TableCell
key={key}
sx={{ fontWeight: 600, fontSize: "1rem" }}
onClick={() => {
handleCellClick(-1, columnIndex);
}}
{...getComponentProps(props, "tableHeadCell")}
>
{label}
</TableCell>
))}
{hasRowActions && (
<TableCell
{...getComponentProps(props, "tableHeadCell", {
sx: {
textAlign: "right",
fontWeight: 600,
fontSize: "1rem",
},
})}
onClick={() => {
handleCellClick(-1, -1);
}}
>
{actions_label || "Actions"}
</TableCell>
)}
</TableRow>
</TableHead>
<TableBody {...getComponentProps(props, "tableBody")}>
{table.map((item) => (
{rows.map((item, rowIndex) => (
<TableRow
key={item.id}
sx={{ "&:last-child td, &:last-child th": { border: 0 } }}
{...getComponentProps(props, "tableBodyRow")}
>
{columns.map(({ key }) => (
<TableCell
key={key}
{...getComponentProps(props, "tableBodyCell")}
>
{item[key]}
{columns.map(({ key }, columnIndex) => {
const coordinate = [rowIndex, columnIndex].join(",");
const isSelected =
selectedCells.has(coordinate) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

I have been running into some of these errors when a tableview does not have selectedRows/Columns/Cells in the initial state.

image

selectedRows.has(rowIndex) ||
selectedColumns.has(columnIndex);
return (
<TableCell
key={key}
sx={{ background: isSelected ? "green" : "unset" }}
onClick={() => {
handleCellClick(rowIndex, columnIndex);
}}
{...getComponentProps(props, "tableBodyCell")}
>
{formatCellValue(item[key], props)}
</TableCell>
);
})}
{hasRowActions && (
<TableCell align="right">
<ActionsMenu actions={getRowActions(rowIndex)} />
</TableCell>
))}
)}
</TableRow>
))}
</TableBody>
Expand All @@ -73,3 +162,56 @@ export default function TableView(props) {
</Box>
);
}

function getTableData(props) {
const { schema, data } = props;
const defaultValue = schema?.default;

if (isAdvancedData(data)) {
return parseAdvancedData(data);
}
if (isAdvancedData(defaultValue)) {
return parseAdvancedData(defaultValue);
}
return {
rows: Array.isArray(data)
? data
: Array.isArray(defaultValue)
? defaultValue
: [],
};
}
Comment on lines +166 to +183
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

LGTM! Initialize selection sets for non-advanced data format.

The getTableData function effectively handles different data formats, including the new advanced data format. The use of helper functions isAdvancedData and parseAdvancedData improves code readability.

However, when the data is not in advanced format, selectedCells, selectedRows, and selectedColumns are not initialized, which could lead to errors. Apply this diff to fix the issue:

 return {
   rows: Array.isArray(data)
     ? data
     : Array.isArray(defaultValue)
     ? defaultValue
     : [],
+  selectedCells: new Set(),
+  selectedRows: new Set(),
+  selectedColumns: new Set(),
 };

This ensures that these sets are always initialized, preventing potential runtime errors.

📝 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
function getTableData(props) {
const { schema, data } = props;
const defaultValue = schema?.default;
if (isAdvancedData(data)) {
return parseAdvancedData(data);
}
if (isAdvancedData(defaultValue)) {
return parseAdvancedData(defaultValue);
}
return {
rows: Array.isArray(data)
? data
: Array.isArray(defaultValue)
? defaultValue
: [],
};
}
function getTableData(props) {
const { schema, data } = props;
const defaultValue = schema?.default;
if (isAdvancedData(data)) {
return parseAdvancedData(data);
}
if (isAdvancedData(defaultValue)) {
return parseAdvancedData(defaultValue);
}
return {
rows: Array.isArray(data)
? data
: Array.isArray(defaultValue)
? defaultValue
: [],
selectedCells: new Set(),
selectedRows: new Set(),
selectedColumns: new Set(),
};
}


function isAdvancedData(data) {
return (
isPlainObject(data) &&
Array.isArray(data?.rows) &&
Array.isArray(data?.columns)
);
}

function parseAdvancedData(data) {
const rows = data.rows.map((row) => {
return data.columns.reduce((cells, column, cellIndex) => {
cells[column] = row[cellIndex];
return cells;
}, {});
});
const selectedCellsRaw = data?.selectedCells || data?.selected_cells || [];
const selectedRowsRaw = data?.selectedRows || data?.selected_rows || [];
const selectedColumnsRaw =
data?.selectedColumns || data?.selected_columns || [];
const selectedCells = new Set(selectedCellsRaw.map((cell) => cell.join(",")));
const selectedRows = new Set(selectedRowsRaw);
const selectedColumns = new Set(selectedColumnsRaw);
return { rows, selectedCells, selectedRows, selectedColumns };
}

function formatCellValue(value: string, props: ViewPropsType) {
const round = props?.schema?.view?.round;
const valueAsFloat = parseFloat(value);
if (!Number.isNaN(valueAsFloat) && typeof round === "number") {
return valueAsFloat.toFixed(round);
}
return value;
}
Loading
Loading