-
Notifications
You must be signed in to change notification settings - Fork 512
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
better icons support in operators #4373
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
import { Icon, IconProps } from "@mui/material"; | ||
import "material-icons/iconfont/material-icons.css"; | ||
import React from "react"; | ||
|
||
// Available Icons: https://github.com/marella/material-icons?tab=readme-ov-file#available-icons | ||
export default function MuiIconFont(props: MuiIconFontProps) { | ||
const { name, ...iconProps } = props; | ||
return <Icon {...iconProps}>{name}</Icon>; | ||
} | ||
|
||
type MuiIconFontProps = IconProps & { | ||
name: string; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
import { MuiIconFont, PillButton } from "@fiftyone/components"; | ||
import usePanelEvent from "@fiftyone/operators/src/usePanelEvent"; | ||
import { usePanelId } from "@fiftyone/spaces"; | ||
import { IconButton, Link } from "@mui/material"; | ||
import React, { useCallback } from "react"; | ||
import styled from "styled-components"; | ||
import { getComponentProps } from "../utils"; | ||
|
||
export default function IconButtonView(props) { | ||
const { schema } = props; | ||
const { view = {} } = schema; | ||
const { icon, operator, params = {}, prompt, variant, label } = view; | ||
const panelId = usePanelId(); | ||
const handleClick = usePanelEvent(); | ||
|
||
const Icon = icon ? ( | ||
<MuiIconFont | ||
name={icon} | ||
sx={ | ||
variant === "round" | ||
? {} | ||
: { color: (theme) => theme.palette.primary.main } | ||
} | ||
{...getComponentProps(props, "icon")} | ||
/> | ||
) : null; | ||
|
||
const onClick = useCallback(() => { | ||
handleClick(panelId, { params, operator, prompt }); | ||
}, [panelId, params, operator, prompt, handleClick]); | ||
|
||
if (variant === "round") { | ||
return ( | ||
<PillButton | ||
icon={Icon || label} | ||
title={label} | ||
highlight | ||
onClick={onClick} | ||
/> | ||
); | ||
} | ||
|
||
if (variant === "square") { | ||
return ( | ||
<SquareButton | ||
title={label} | ||
aria-label={icon} | ||
onClick={onClick} | ||
{...getComponentProps(props, "button")} | ||
> | ||
{Icon || label} | ||
</SquareButton> | ||
); | ||
} | ||
|
||
return ( | ||
<IconButton | ||
title={label} | ||
aria-label={icon} | ||
{...getComponentProps(props, "button")} | ||
onClick={onClick} | ||
> | ||
{Icon || label} | ||
</IconButton> | ||
); | ||
} | ||
|
||
const SquareButton = styled(Link)` | ||
display: flex; | ||
color: var(--fo-palette-primary-plainColor); | ||
align-items: center; | ||
cursor: pointer; | ||
border-bottom: 1px var(--fo-palette-primary-plainColor) solid; | ||
background: var(--fo-palette-neutral-softBg); | ||
border-top-left-radius: 3px; | ||
border-top-right-radius: 3px; | ||
padding: 0.25rem; | ||
`; |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,6 +1,7 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { usePluginDefinition } from "@fiftyone/plugins"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { useColorScheme } from "@mui/material"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { IconProps, useColorScheme } from "@mui/material"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { resolveServerPath } from "./utils"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { MuiIconFont } from "@fiftyone/components"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
export default function OperatorIcon(props: CustomIconPropsType) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -11,26 +12,43 @@ export default function OperatorIcon(props: CustomIconPropsType) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
_builtIn, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Fallback, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
canExecute, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
iconProps, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} = props; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const { mode } = useColorScheme(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const iconPath = mode === "dark" && darkIcon ? darkIcon : lightIcon || icon; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (isIconFont(iconPath)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return <MuiIconFont {...(iconProps as IconProps)} name={iconPath} />; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (!iconPath || !canExecute) return Fallback ? <Fallback /> : null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (_builtIn) return <ImageIcon src={iconPath} />; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return <CustomOperatorIcon pluginName={pluginName} iconPath={iconPath} />; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
<CustomOperatorIcon | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
pluginName={pluginName} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
iconPath={iconPath} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
iconProps={iconProps as JSX.IntrinsicElements["img"]} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Refactor to improve readability and maintainability. The conditional logic for rendering different types of icons can be simplified to improve readability and maintainability. - if (isIconFont(iconPath)) {
- return <MuiIconFont {...(iconProps as IconProps)} name={iconPath} />;
- }
- if (!iconPath || !canExecute) return Fallback ? <Fallback /> : null;
- if (_builtIn) return <ImageIcon src={iconPath} />;
- return (
- <CustomOperatorIcon
- pluginName={pluginName}
- iconPath={iconPath}
- iconProps={iconProps as JSX.IntrinsicElements["img"]}
- />
- );
+ if (!iconPath || !canExecute) return Fallback ? <Fallback /> : null;
+ if (isIconFont(iconPath)) {
+ return <MuiIconFont {...(iconProps as IconProps)} name={iconPath} />;
+ } else if (_builtIn) {
+ return <ImageIcon src={iconPath} />;
+ } else {
+ return (
+ <CustomOperatorIcon
+ pluginName={pluginName}
+ iconPath={iconPath}
+ iconProps={iconProps as JSX.IntrinsicElements["img"]}
+ />
+ );
+ } Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
function CustomOperatorIcon(props: CustomOperatorIconPropsType) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const { pluginName, iconPath } = props; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const { pluginName, iconPath, iconProps } = props; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const plugin = usePluginDefinition(pluginName); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const assetPath = resolveServerPath(plugin); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const resolvedIconPath = assetPath + iconPath; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return <ImageIcon src={resolvedIconPath} />; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return <ImageIcon src={resolvedIconPath} iconProps={iconProps} />; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
function ImageIcon(props: ImageIconPropsType) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const { src } = props; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return <img src={src} height={21} width={21} />; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const { src, iconProps } = props; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return <img {...iconProps} src={src} height={21} width={21} />; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
function isIconFont(path: string) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
typeof path === "string" && !path?.includes("/") && !path?.includes(".") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
export type CustomIconPropsType = { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -41,13 +59,16 @@ export type CustomIconPropsType = { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
_builtIn?: boolean; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Fallback?: React.ComponentType; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
canExecute?: boolean; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
iconProps?: IconProps | JSX.IntrinsicElements["img"]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
type CustomOperatorIconPropsType = { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
pluginName?: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
iconPath?: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
iconProps?: JSX.IntrinsicElements["img"]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
type ImageIconPropsType = { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
src: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
iconProps?: JSX.IntrinsicElements["img"]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
import { PluginComponentType, registerComponent } from "@fiftyone/plugins"; | ||
import { defineCustomPanel } from "../CustomPanel"; | ||
import OperatorIcon from "../OperatorIcon"; | ||
|
||
export default function registerPanel(ctx) { | ||
registerComponent({ | ||
type: PluginComponentType.Panel, | ||
name: ctx.params.panel_name, | ||
component: defineCustomPanel(ctx.params), | ||
label: ctx.params.panel_label, | ||
activator: () => true, | ||
Icon: () => { | ||
return ( | ||
<OperatorIcon | ||
icon={ctx.params.icon || "extension"} | ||
darkIcon={ctx.params.dark_icon} | ||
lightIcon={ctx.params.light_icon} | ||
iconProps={{ sx: { fontSize: 14, mr: "0.5rem" } }} | ||
/> | ||
); | ||
}, | ||
panelOptions: { | ||
allowDuplicates: ctx.params.allow_duplicates, | ||
}, | ||
}); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tip Codebase Verification The review comment about the lack of error handling in the To address this, it would be beneficial to implement error handling mechanisms such as:
This will help prevent potential runtime errors and make the function more robust. Analysis chainEnsure proper error handling in While the function implementation looks correct, it's crucial to ensure that error handling is in place for cases where Scripts ExecutedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify error handling in the `registerPanel` function.
# Test: Search for error handling related to `ctx.params`.
rg --type ts "ctx.params" "app/packages/operators/src/Panel/register.tsx"
Length of output: 371 |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,7 +1,6 @@ | ||||||||||||||||||||
import { | ||||||||||||||||||||
Layout, | ||||||||||||||||||||
SpaceNode, | ||||||||||||||||||||
usePanel, | ||||||||||||||||||||
usePanelState, | ||||||||||||||||||||
usePanels, | ||||||||||||||||||||
useSetPanelStateById, | ||||||||||||||||||||
|
@@ -13,9 +12,11 @@ import * as types from "./types"; | |||||||||||||||||||
|
||||||||||||||||||||
import { toSlug } from "@fiftyone/utilities"; | ||||||||||||||||||||
import copyToClipboard from "copy-to-clipboard"; | ||||||||||||||||||||
import { merge } from "lodash"; | ||||||||||||||||||||
import { useSetRecoilState } from "recoil"; | ||||||||||||||||||||
import { useOperatorExecutor } from "."; | ||||||||||||||||||||
import useRefetchableSavedViews from "../../core/src/hooks/useRefetchableSavedViews"; | ||||||||||||||||||||
import registerPanel from "./Panel/register"; | ||||||||||||||||||||
import { | ||||||||||||||||||||
ExecutionContext, | ||||||||||||||||||||
Operator, | ||||||||||||||||||||
|
@@ -25,10 +26,7 @@ import { | |||||||||||||||||||
executeOperator, | ||||||||||||||||||||
listLocalAndRemoteOperators, | ||||||||||||||||||||
} from "./operators"; | ||||||||||||||||||||
import { usePromptOperatorInput, useShowOperatorIO } from "./state"; | ||||||||||||||||||||
import { merge } from "lodash"; | ||||||||||||||||||||
import { PluginComponentType, registerComponent } from "@fiftyone/plugins"; | ||||||||||||||||||||
import CustomPanel, { defineCustomPanel } from "./CustomPanel"; | ||||||||||||||||||||
import { useShowOperatorIO } from "./state"; | ||||||||||||||||||||
import usePanelEvent from "./usePanelEvent"; | ||||||||||||||||||||
|
||||||||||||||||||||
// | ||||||||||||||||||||
|
@@ -982,16 +980,7 @@ class RegisterPanel extends Operator { | |||||||||||||||||||
return new types.Property(inputs); | ||||||||||||||||||||
} | ||||||||||||||||||||
async execute(ctx: ExecutionContext): Promise<void> { | ||||||||||||||||||||
registerComponent({ | ||||||||||||||||||||
type: PluginComponentType.Panel, | ||||||||||||||||||||
name: ctx.params.panel_name, | ||||||||||||||||||||
component: defineCustomPanel(ctx.params), | ||||||||||||||||||||
label: ctx.params.panel_label, | ||||||||||||||||||||
activator: () => true, | ||||||||||||||||||||
panelOptions: { | ||||||||||||||||||||
allowDuplicates: ctx.params.allow_duplicates, | ||||||||||||||||||||
}, | ||||||||||||||||||||
}); | ||||||||||||||||||||
registerPanel(ctx); | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The use of - registerPanel(ctx);
+ registerPanel({
+ name: ctx.params.panel_name,
+ label: ctx.params.panel_label,
+ on_load: ctx.params.on_load,
+ on_change: ctx.params.on_change,
+ on_unload: ctx.params.on_unload,
+ allow_duplicates: ctx.params.allow_duplicates
+ }); Committable suggestion
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The parameters are extracted and used in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @imanjra, thank you for your patience. After reviewing the implementation of the I appreciate your clarification, and I'll remember this detail for future reviews. Analysis chainScripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for the implementation of the `registerPanel` function in TypeScript files
rg "function registerPanel" --type ts
Length of output: 130 Script: #!/bin/bash
# Fetch and display the content of register.tsx where registerPanel is defined
cat app/packages/operators/src/Panel/register.tsx
Length of output: 833 New Learnings Added
|
||||||||||||||||||||
} | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🆒