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

Add PillBadgeView to Python Panels #4909

Merged
merged 13 commits into from
Oct 10, 2024
140 changes: 140 additions & 0 deletions app/packages/components/src/components/PillBadge/PillBadge.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
import React, { useState } from "react";
import CircleIcon from "@mui/icons-material/Circle";
import { Chip, FormControl, MenuItem, Select } from "@mui/material";

const PillBadge = ({
text,
color = "default",
variant = "filled",
showIcon = true,
}: {
text: string | string[] | [string, string][];
color?: string;
variant?: "filled" | "outlined";
showIcon?: boolean;
}) => {
Comment on lines +5 to +15
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

Consider simplifying the text prop to improve maintainability.

The text prop accepts multiple types (string, string[], and [string, string][]), which increases complexity in the component logic and can lead to potential errors. Simplifying the prop to accept a single data type or splitting the component into separate components for each case could enhance readability and maintainability.

const [chipSelection, setChipSelection] = useState(
typeof text === "string"
? text
: Array.isArray(text)
? Array.isArray(text[0])
? text[0][0]
: text[0]
: ""
);
const [chipColor, setChipColor] = useState(
typeof text === "string"
? color
: Array.isArray(text)
? Array.isArray(text[0])
? text[0][1]
: color || "default"
: "default"
);
Br2850 marked this conversation as resolved.
Show resolved Hide resolved

const COLORS: { [key: string]: string } = {
default: "#999999",
primary: "#FFB682",
error: "error",
warning: "warning",
info: "info",
success: "#8BC18D",
};
Comment on lines +44 to +51
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

Ensure COLORS object contains valid CSS color values.

In the COLORS object, the values for 'error', 'warning', and 'info' are strings that may not correspond to valid CSS color values. Assigning these strings to the color style property may not produce the desired effect. Consider replacing them with appropriate color codes or ensuring that the component interprets these string values correctly.

For example, you can update the COLORS object with valid color codes:

 const COLORS: { [key: string]: string } = {
   default: "#999999",
   primary: "#FFB682",
-  error: "error",
-  warning: "warning",
-  info: "info",
+  error: "#f44336",
+  warning: "#ff9800",
+  info: "#2196f3",
   success: "#8BC18D",
 };
📝 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
const COLORS: { [key: string]: string } = {
default: "#999999",
primary: "#FFB682",
error: "error",
warning: "warning",
info: "info",
success: "#8BC18D",
};
const COLORS: { [key: string]: string } = {
default: "#999999",
primary: "#FFB682",
error: "#f44336",
warning: "#ff9800",
info: "#2196f3",
success: "#8BC18D",
};


const chipStyle: { [key: string]: string | number } = {
color: COLORS[chipColor || "default"] || COLORS.default,
fontWeight: 500,
paddingLeft: 1,
};

return (
<span>
{typeof text === "string" ? (
<Chip
icon={
showIcon ? (
<CircleIcon color={"inherit"} sx={{ fontSize: 10 }} />
) : undefined
}
label={text}
sx={{
...chipStyle,
"& .MuiChip-icon": {
marginRight: "-7px",
},
"& .MuiChip-label": {
marginBottom: "1px",
},
}}
variant={variant as "filled" | "outlined" | undefined}
/>
) : (
<FormControl fullWidth>
<Chip
icon={
showIcon ? (
<CircleIcon color={"inherit"} sx={{ fontSize: 10 }} />
) : undefined
}
label={
Array.isArray(text) && Array.isArray(text[0]) ? (
<Select
value={chipSelection}
variant={"standard"}
disableUnderline={true}
onChange={(event) => {
const selectedText = text.find(
(t) => t[0] === event.target.value
);
setChipSelection(event.target.value);
setChipColor(selectedText?.[1] ?? "default");
}}
Comment on lines +94 to +100
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

Add checks for selectedText to prevent undefined access.

In the onChange handler, selectedText may be undefined if text.find does not find a matching element. Accessing selectedText[1] without checking if selectedText exists could lead to runtime errors. Consider adding a check to ensure selectedText is defined before using it.

Here's an example of how to adjust the code:

 onChange={(event) => {
   const selectedText = text.find(
     (t) => t[0] === event.target.value
   );
   setChipSelection(event.target.value);
-  setChipColor(selectedText?.[1] ?? "default");
+  if (selectedText) {
+    setChipColor(selectedText[1]);
+  } else {
+    setChipColor("default");
+  }
 }}
📝 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
onChange={(event) => {
const selectedText = text.find(
(t) => t[0] === event.target.value
);
setChipSelection(event.target.value);
setChipColor(selectedText?.[1] ?? "default");
}}
onChange={(event) => {
const selectedText = text.find(
(t) => t[0] === event.target.value
);
setChipSelection(event.target.value);
if (selectedText) {
setChipColor(selectedText[1]);
} else {
setChipColor("default");
}
}}

sx={{
color: "inherit",
}}
>
{text.map((t, index) => (
<MenuItem key={index} value={t[0]}>
{t[0]}
</MenuItem>
))}
</Select>
Br2850 marked this conversation as resolved.
Show resolved Hide resolved
) : (
<Select
value={chipSelection}
variant={"standard"}
disableUnderline={true}
onChange={(event) => setChipSelection(event.target.value)}
sx={{
color: "inherit",
}}
>
{text.map((t, index) => (
<MenuItem key={index} value={t}>
{t}
</MenuItem>
))}
</Select>
)
}
sx={{
...chipStyle,
"& .MuiChip-icon": {
marginRight: "-7px",
},
"& .MuiChip-label": {
marginBottom: "1px",
},
"& .MuiInput-input:focus": {
backgroundColor: "inherit",
},
}}
variant={variant as "filled" | "outlined" | undefined}
></Chip>
</FormControl>
)}
</span>
);
};

export default PillBadge;
1 change: 1 addition & 0 deletions app/packages/components/src/components/PillBadge/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { default as PillBadge } from "./PillBadge";
2 changes: 1 addition & 1 deletion app/packages/core/src/components/Modal/use-looker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ function useLooker<L extends fos.Lookers>({
colorScheme;
/** end refreshers */

!initialRef.current && looker.updateSample(sample);
!initialRef.current && looker.updateSample(sample.sample);
}, [colorScheme, looker, sample]);

useEffect(() => {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { Box } from "@mui/material";
import React from "react";
import { getComponentProps } from "../utils";
import PillBadge from "@fiftyone/components/src/components/PillBadge/PillBadge";

export default function PillBadgeView(props) {
const { schema } = props;
const { view = {} } = schema;
const { text, color, variant, showIcon } = view;
Comment on lines +6 to +9
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

Consider adding TypeScript types and using a named export.

While the component structure is good, it could benefit from the following improvements:

  1. Add TypeScript type annotations for props and destructured values to enhance type safety and developer experience.
  2. Consider using a named export instead of a default export for better refactoring capabilities and explicit imports.

Here's a suggested refactor:

import { Box } from "@mui/material";
import React from "react";
import { getComponentProps } from "../utils";
import PillBadge from "@fiftyone/components/src/components/PillBadge/PillBadge";

interface PillBadgeViewProps {
  schema: {
    view?: {
      text?: string;
      color?: string;
      variant?: string;
      showIcon?: boolean;
    };
  };
}

export function PillBadgeView({ schema }: PillBadgeViewProps) {
  const { view = {} } = schema;
  const { text, color, variant, showIcon } = view;

  // ... rest of the component
}


return (
<Box {...getComponentProps(props, "container")}>
<PillBadge
text={text}
color={color}
variant={variant}
showIcon={showIcon}
{...getComponentProps(props, "pillBadge")}
/>
</Box>
);
}
1 change: 1 addition & 0 deletions app/packages/core/src/plugins/SchemaIO/components/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,3 +49,4 @@ export { default as TextFieldView } from "./TextFieldView";
export { default as TupleView } from "./TupleView";
export { default as UnsupportedView } from "./UnsupportedView";
export { default as FrameLoaderView } from "./FrameLoaderView";
export { default as PillBadgeView } from "./PillBadgeView";
2 changes: 1 addition & 1 deletion e2e-pw/src/oss/specs/sidebar/frame-filtering.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ test.describe("frame filtering", () => {
for name, disable in datasets:
dataset = foz.load_zoo_dataset("quickstart-video", dataset_name=name, max_samples=1)
dataset.app_config.disable_frame_filtering = disable
dataset.persist = True
dataset.persistent = True
dataset.save()
`
);
Expand Down
14 changes: 14 additions & 0 deletions fiftyone/operators/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -1475,6 +1475,20 @@ def __init__(self, **kwargs):
super().__init__(**kwargs)


class PillBadgeView(ReadOnlyView):
"""Displays a pill shaped badge.

Args:
text ("Reviewed" | ["Reviewed", "Not Reviewed"] | [["Not Started", "primary"], ["Reviewed", "success"], ["In Review", "warning"]): a label or set of label options with or without a color for the pill badge
color ("primary"): the color of the pill
variant ("outlined"): the variant of the pill
show_icon (False | True): whether to display indicator icon
"""

def __init__(self, **kwargs):
super().__init__(**kwargs)


class PlotlyView(View):
"""Displays a Plotly chart.

Expand Down
9 changes: 7 additions & 2 deletions fiftyone/utils/sam.py
Original file line number Diff line number Diff line change
Expand Up @@ -271,14 +271,19 @@ def _forward_pass_boxes(self, imgs):
device=sam_predictor.device,
)

masks, _, _ = sam_predictor.predict_torch(
masks, scores, _ = sam_predictor.predict_torch(
point_coords=None,
point_labels=None,
boxes=transformed_boxes,
multimask_output=False,
)
outputs.append(
{"boxes": input_boxes, "labels": labels, "masks": masks}
{
"boxes": input_boxes,
"labels": labels,
"masks": masks,
"scores": scores,
}
)

return outputs
Expand Down
5 changes: 4 additions & 1 deletion fiftyone/utils/sam2.py
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ def _forward_pass_boxes(self, imgs):
device=sam2_predictor.device,
)

masks, _, _ = sam2_predictor.predict(
masks, scores, _ = sam2_predictor.predict(
point_coords=None,
point_labels=None,
box=sam_boxes[None, :],
Expand All @@ -214,6 +214,9 @@ def _forward_pass_boxes(self, imgs):
"boxes": input_boxes,
"labels": labels,
"masks": torch.tensor(masks, device=sam2_predictor.device),
"scores": torch.tensor(
scores, device=sam2_predictor.device
),
}
)

Expand Down
Loading