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

additional panel meta (is_new, beta, category) #5038

Merged
merged 6 commits into from
Nov 4, 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
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ let theme = extendMuiTheme({
shadowDark: "hsl(200, 0%, 70%)",
lightning: "hsl(25, 100%, 51%)",
toastBackgroundColor: "#FFFFFF",
primarySoft: "hsl(25, 100%, 51%)",
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

Add TypeScript interface for custom colors.

The new primarySoft color property should be properly typed in the theme's TypeScript interface to maintain type safety.

Add this interface update at the top of the file:

declare module '@mui/material/styles' {
  interface CustomPaletteColor {
    primarySoft?: string;
  }
}

Also applies to: 181-181

},
voxel: {
500: "#FF6D04",
Expand Down Expand Up @@ -177,6 +178,7 @@ let theme = extendMuiTheme({
shadowDark: "hsl(200, 0%, 0%)",
lightning: "#f5b700",
toastBackgroundColor: "#333",
primarySoft: "hsl(25, 100%, 80%)",
},
voxel: {
500: "#FF6D04",
Expand Down
7 changes: 6 additions & 1 deletion app/packages/core/src/plugins/histograms.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
import { Loading, Selector } from "@fiftyone/components";
import { OperatorPlacements, types } from "@fiftyone/operators";
import { PluginComponentType, registerComponent } from "@fiftyone/plugins";
import {
Categories,
PluginComponentType,
registerComponent,
} from "@fiftyone/plugins";
import { usePanelTitle } from "@fiftyone/spaces";
import { datasetName, distributionPaths, field } from "@fiftyone/state";
import { BarChart } from "@mui/icons-material";
Expand Down Expand Up @@ -108,5 +112,6 @@ registerComponent({
Icon: BarChart,
panelOptions: {
priority: BUILT_IN_PANEL_PRIORITY_CONST,
category: Categories.Curate,
},
});
6 changes: 5 additions & 1 deletion app/packages/embeddings/src/EmptyEmbeddings.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,11 @@ export default function EmptyEmbeddings() {
<Grid container direction="column" alignItems="center" spacing={2}>
<Grid item>
<WorkspacesIcon
sx={{ fontSize: 64, color: "#ffba85", marginBottom: 2 }}
sx={{
fontSize: 64,
color: theme.custom.primarySoft,
marginBottom: 2,
}}
/>
</Grid>
<Grid item>
Expand Down
7 changes: 6 additions & 1 deletion app/packages/embeddings/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
import { PluginComponentType, registerComponent } from "@fiftyone/plugins";
import {
Categories,
PluginComponentType,
registerComponent,
} from "@fiftyone/plugins";
import WorkspacesIcon from "@mui/icons-material/Workspaces";
import Embeddings from "./Embeddings";
import EmbeddingsTabIndicator from "./EmbeddingsTabIndicator";
Expand All @@ -14,5 +18,6 @@ registerComponent({
panelOptions: {
TabIndicator: EmbeddingsTabIndicator,
priority: BUILT_IN_PANEL_PRIORITY_CONST,
category: Categories.Curate,
},
});
4 changes: 4 additions & 0 deletions app/packages/operators/src/Panel/register.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ export default function registerPanel(ctx: ExecutionContext) {
helpMarkdown: ctx.params.help_markdown,
reloadOnNavigation: ctx.params.reload_on_navigation,
surfaces: ctx.params.surfaces,
beta: ctx.params.beta,
category: ctx.params.category,
isNew: ctx.params.is_new,
priority: ctx.params.priority,
},
});
}
5 changes: 5 additions & 0 deletions app/packages/operators/src/built-in-operators.ts
Original file line number Diff line number Diff line change
Expand Up @@ -812,7 +812,7 @@
unlisted: true,
});
}
useHooks(ctx: ExecutionContext): {} {

Check warning on line 815 in app/packages/operators/src/built-in-operators.ts

View workflow job for this annotation

GitHub Actions / lint / eslint

'ctx' is defined but never used. Allowed unused args must match /^_/u
return { updatePanelState: useUpdatePanelStatePartial() };
}
async execute(ctx: ExecutionContext): Promise<void> {
Expand Down Expand Up @@ -997,6 +997,10 @@
inputs.str("panel_name", { label: "Panel name", required: true });
inputs.str("panel_label", { label: "Panel label", required: true });
inputs.str("icon", { label: "Icon" });
inputs.str("help_markdown", { label: "Help markdown" });
inputs.str("category", { label: "Category" });
inputs.bool("beta", { label: "Beta", default: false });
inputs.bool("is_new", { label: "NEW", default: false });
inputs.str("dark_icon", { label: "Icon for dark mode" });
inputs.str("light_icon", { label: "Icon for light mode" });
inputs.str("on_load", { label: "On load operator" });
Expand All @@ -1006,6 +1010,7 @@
label: "Allow duplicates",
default: false,
});
inputs.int("priority", { label: "Priority" });
return new types.Property(inputs);
}
async execute(ctx: ExecutionContext): Promise<void> {
Expand Down
46 changes: 46 additions & 0 deletions app/packages/plugins/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,37 @@ export enum PluginComponentType {
*/
}

type CategoryID = "import" | "curate" | "analyze" | "custom";
Copy link
Contributor

@Br2850 Br2850 Nov 4, 2024

Choose a reason for hiding this comment

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

Given that @brimoor this was your original vision for the categories of workflows with Assistant, do we want to simply just add all of these (Explore, Annotate, Evaluate, etc.) right now?

Not sure if this list is still the "right" list in terms of categorization, but food for thought @ritch

Screenshot 2024-11-04 at 3 13 35 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We've been discussing the list in a slack thread


export enum Categories {
Import = "import",
Curate = "curate",
Analyze = "analyze",
Custom = "custom",
}

export function getCategoryLabel(category: CategoryID): string {
switch (category) {
case "import":
return "Import";
case "curate":
return "Curate";
case "analyze":
return "Analyze";
case "custom":
return "Custom";
}
}

export function getCategoryForPanel(panel: PluginComponentRegistration) {
return panel.panelOptions?.category || "custom";
}

type Category = {
id: CategoryID;
label: string;
};

type PluginActivator = (props: any) => boolean;

type PanelOptions = {
Expand Down Expand Up @@ -331,6 +362,21 @@ type PanelOptions = {
* This is only applicable to plugins that are in a modal.
*/
reloadOnNavigation?: boolean;

/**
* The category of the plugin
*/
category: CategoryID;

/**
* Whether the plugin is in beta
*/
beta: boolean;

/**
* Whether the plugin is new
*/
isNew: boolean;
Comment on lines +365 to +379
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

Consider making the category property optional.

The category property is currently required, which could break existing panel implementations. Since getCategoryForPanel provides a default value of "custom", consider making this property optional for better backward compatibility.

 type PanelOptions = {
   // ... existing properties ...

   /**
    * The category of the plugin
    */
-  category: CategoryID;
+  category?: CategoryID;

   /**
    * Whether the plugin is in beta
    */
   beta: boolean;

   /**
    * Whether the plugin is new
    */
   isNew: boolean;
 };
📝 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
/**
* The category of the plugin
*/
category: CategoryID;
/**
* Whether the plugin is in beta
*/
beta: boolean;
/**
* Whether the plugin is new
*/
isNew: boolean;
/**
* The category of the plugin
*/
category?: CategoryID;
/**
* Whether the plugin is in beta
*/
beta: boolean;
/**
* Whether the plugin is new
*/
isNew: boolean;

};

type PluginComponentProps<T> = T & {
Expand Down
84 changes: 67 additions & 17 deletions app/packages/spaces/src/components/AddPanelButton.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
import { IconButton, Popout, scrollable } from "@fiftyone/components";
import { PluginComponentRegistration } from "@fiftyone/plugins";
import { IconButton, Popout, scrollable, useTheme } from "@fiftyone/components";
import {
Categories,
getCategoryForPanel,
getCategoryLabel,
PluginComponentRegistration,
} from "@fiftyone/plugins";
import * as fos from "@fiftyone/state";
import { Add } from "@mui/icons-material";
import { useCallback, useMemo, useRef, useState } from "react";
Expand All @@ -9,6 +14,7 @@ import { AddPanelButtonProps } from "../types";
import { panelsCompareFn } from "../utils/sort";
import AddPanelItem from "./AddPanelItem";
import { AddPanelButtonContainer } from "./StyledElements";
import { Typography, Grid } from "@mui/material";

export default function AddPanelButton({ node, spaceId }: AddPanelButtonProps) {
const [open, setOpen] = useState(false);
Expand All @@ -26,9 +32,7 @@ export default function AddPanelButton({ node, spaceId }: AddPanelButtonProps) {
const panels = usePanels(panelsPredicate);
const spaceNodes = useSpaceNodes(spaceId);
const nodeTypes = useMemo(() => {
return spaceNodes.map((node) => {
return node.type;
});
return spaceNodes.map((node) => node.type);
}, [spaceNodes]);
const popoutRef = useRef();
fos.useOutsideClick(popoutRef, () => {
Expand All @@ -44,14 +48,28 @@ export default function AddPanelButton({ node, spaceId }: AddPanelButtonProps) {
})
.sort(panelsCompareFn);

const categorizedPanels = availablePanels.reduce((acc, panel) => {
const category = getCategoryForPanel(panel);
if (!acc[category]) {
acc[category] = {
label: getCategoryLabel(category),
panels: [],
};
}
acc[category].panels.push(panel);
return acc;
}, {});

if (availablePanels.length === 0) return null;

const sortedCategories = ["import", "curate", "analyze", "custom"]
.map((cat) => categorizedPanels[cat])
.filter((c) => c);

return (
<AddPanelButtonContainer ref={popoutRef}>
<IconButton
onClick={() => {
setOpen(!open);
}}
onClick={() => setOpen(!open)}
title="New panel"
data-cy="new-panel-btn"
>
Expand All @@ -68,17 +86,49 @@ export default function AddPanelButton({ node, spaceId }: AddPanelButtonProps) {
}}
popoutProps={{ className: scrollable }}
>
{availablePanels.map((panel) => (
<AddPanelItem
key={panel.name}
spaceId={spaceId}
{...panel}
node={node}
onClick={() => setOpen(!open)}
/>
))}
<PanelCategories>
{sortedCategories.map(({ label, panels }) => (
<PanelCategory key={label} label={label}>
{panels.map((panel) => (
<AddPanelItem
key={panel.name}
node={node}
name={panel.name}
label={panel.label}
spaceId={spaceId}
showBeta={panel.panelOptions?.beta}
showNew={panel.panelOptions?.isNew}
onClick={() => setOpen(false)}
/>
))}
</PanelCategory>
))}
</PanelCategories>
</Popout>
)}
</AddPanelButtonContainer>
);
}

function PanelCategories({ children }) {
return (
<Grid container gap={1} sx={{ p: 2 }}>
{children}
</Grid>
);
}

function PanelCategory({ label, children }) {
const theme = useTheme();
return (
<Grid item>
<Typography
variant="subtitle2"
sx={{ padding: "0 8px", color: theme.text.secondary }}
>
{label || "no category"}
</Typography>
{children}
</Grid>
);
}
26 changes: 26 additions & 0 deletions app/packages/spaces/src/components/AddPanelItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,18 @@ import { useSpaces } from "../hooks";
import SpaceNode from "../SpaceNode";
import { AddPanelItemProps } from "../types";
import PanelIcon from "./PanelIcon";
import { useTheme } from "@fiftyone/components";

export default function AddPanelItem({
node,
name,
label,
onClick,
spaceId,
showBeta,
showNew,
}: AddPanelItemProps) {
const theme = useTheme();
const trackEvent = useTrackEvent();
const { spaces } = useSpaces(spaceId);
return (
Expand Down Expand Up @@ -48,6 +52,28 @@ export default function AddPanelItem({
>
{label || (name as string)}
</Typography>
{showNew && (
<span
style={{
color: theme.custom.primarySoft,
fontSize: "11px",
marginLeft: "6px",
}}
>
NEW
</span>
)}
{showBeta && (
<span
style={{
color: theme.custom.primarySoft,
fontSize: "11px",
marginLeft: "6px",
}}
>
BETA
</span>
)}
Comment on lines +55 to +76
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

Refactor duplicate label styling logic.

The NEW and BETA label implementations share identical styling logic, which violates the DRY principle.

Consider extracting the shared styling into a reusable component:

+const StatusLabel = ({ children }: { children: React.ReactNode }) => {
+  const theme = useTheme();
+  return (
+    <span
+      style={{
+        color: theme.custom.primarySoft,
+        fontSize: "11px",
+        marginLeft: "6px",
+      }}
+    >
+      {children}
+    </span>
+  );
+};

 // In the main component
-{showNew && (
-  <span
-    style={{
-      color: theme.custom.primarySoft,
-      fontSize: "11px",
-      marginLeft: "6px",
-    }}
-  >
-    NEW
-  </span>
-)}
-{showBeta && (
-  <span
-    style={{
-      color: theme.custom.primarySoft,
-      fontSize: "11px",
-      marginLeft: "6px",
-    }}
-  >
-    BETA
-  </span>
-)}
+{showNew && <StatusLabel>NEW</StatusLabel>}
+{showBeta && <StatusLabel>BETA</StatusLabel>}

Additionally, consider these accessibility improvements:

  1. Add appropriate ARIA labels
  2. Use semantic HTML elements

Consider enhancing accessibility:

 const StatusLabel = ({ children }: { children: React.ReactNode }) => {
   const theme = useTheme();
   return (
     <span
+      role="status"
+      aria-label={`${children.toString().toLowerCase()} feature`}
       style={{
         color: theme.custom.primarySoft,
         fontSize: "11px",
         marginLeft: "6px",
       }}
     >
       {children}
     </span>
   );
 };
📝 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
{showNew && (
<span
style={{
color: theme.custom.primarySoft,
fontSize: "11px",
marginLeft: "6px",
}}
>
NEW
</span>
)}
{showBeta && (
<span
style={{
color: theme.custom.primarySoft,
fontSize: "11px",
marginLeft: "6px",
}}
>
BETA
</span>
)}
const StatusLabel = ({ children }: { children: React.ReactNode }) => {
const theme = useTheme();
return (
<span
role="status"
aria-label={`${children.toString().toLowerCase()} feature`}
style={{
color: theme.custom.primarySoft,
fontSize: "11px",
marginLeft: "6px",
}}
>
{children}
</span>
);
};
{showNew && <StatusLabel>NEW</StatusLabel>}
{showBeta && <StatusLabel>BETA</StatusLabel>}

</Stack>
);
}
16 changes: 9 additions & 7 deletions app/packages/spaces/src/components/PanelIcon.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import ExtensionIcon from "@mui/icons-material/Extension";
import { usePanel } from "../hooks";
import { PanelIconProps } from "../types";
import { warnPanelNotFound } from "../utils";
import { Box } from "@mui/material";

export default function PanelIcon(props: PanelIconProps) {
const { name } = props;
Expand All @@ -10,12 +11,13 @@ export default function PanelIcon(props: PanelIconProps) {
const { Icon } = panel;
const PanelTabIcon = Icon || ExtensionIcon;
return (
<PanelTabIcon
style={{
width: "1rem",
height: "1rem",
marginRight: "0.5rem",
}}
/>
<Box sx={{ mr: "0.75rem", width: "1rem", height: "1.5rem" }}>
<PanelTabIcon
style={{
width: "1rem",
height: "1rem",
}}
/>
</Box>
);
}
Loading
Loading