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.

Sign up for GitHub

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 exec button to py panels #5119

Merged
merged 2 commits into from
Nov 15, 2024
Merged

add exec button to py panels #5119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c147a99
add exec button to py panels
ritch Nov 15, 2024
c755a45
add exec button ts type
ritch Nov 15, 2024
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
13 changes: 6 additions & 7 deletions app/packages/core/src/plugins/SchemaIO/components/OperatorExecutionButtonView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import ExpandMoreIcon from "@mui/icons-material/ExpandMore";
import TooltipProvider from "./TooltipProvider";

export default function OperatorExecutionButtonView(props: ViewPropsType) {
const { schema, path, onClick } = props;
const { schema, path } = props;
const { view = {} } = schema;
const {
description,
Expand All @@ -19,7 +19,6 @@ export default function OperatorExecutionButtonView(props: ViewPropsType) {
label,
operator,
params = {},
prompt,
title,
disabled = false,
} = view;
Expand All @@ -43,8 +42,6 @@ export default function OperatorExecutionButtonView(props: ViewPropsType) {
operatorUri={operator}
executionParams={computedParams}
variant={variant}
onClick={(e) => onClick?.(e, computedParams, props)}
color="primary"
disabled={disabled}
startIcon={icon_position === "left" ? Icon : undefined}
endIcon={icon_position === "right" ? Icon : undefined}
Expand All @@ -59,7 +56,8 @@ export default function OperatorExecutionButtonView(props: ViewPropsType) {
}

function getButtonProps(props: ViewPropsType): ButtonProps {
const { label, variant, color, disabled } = props.schema.view;
const { label, color, disabled } = props.schema.view;
const variant = getVariant(props);
const baseProps: ButtonProps = getCommonProps(props);
if (isNullish(label)) {
baseProps.sx["& .MuiButton-startIcon"] = { mr: 0, ml: 0 };
Expand All @@ -86,9 +84,10 @@ function getButtonProps(props: ViewPropsType): ButtonProps {
baseProps.sx.borderColor = borderColor;
baseProps.sx.borderBottomColor = borderColor;
}
if (isNullish(variant)) {
if (isNullish(variant) || variant === "contained") {
baseProps.variant = "contained";
baseProps.color = "tertiary";
baseProps.color = "primary";
baseProps.sx.color = (theme) => theme.palette.text.primary;
baseProps.sx["&:hover"] = {
backgroundColor: (theme) => theme.palette.tertiary.hover,
};
Expand Down
14 changes: 14 additions & 0 deletions app/packages/operators/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1241,6 +1241,20 @@ export class ToastView extends View {
}
}

/**
* Operator class for rendering a execution button.
*/

class OperatorExecutionButtonView extends View {
constructor(options: ViewProps) {
super(options);
this.name = "OperatorExecutionButtonView";
}
static fromJSON(json) {
return new OperatorExecutionButtonView(json);
}
}
Comment on lines +1248 to +1256
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

Export the OperatorExecutionButtonView class.

The class should be exported to make it available for use in other modules, following the pattern of other view classes in this file.

-class OperatorExecutionButtonView extends View {
+export class OperatorExecutionButtonView extends View {
📝 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
class OperatorExecutionButtonView extends View {
constructor(options: ViewProps) {
super(options);
this.name = "OperatorExecutionButtonView";
}
static fromJSON(json) {
return new OperatorExecutionButtonView(json);
}
}
export class OperatorExecutionButtonView extends View {
constructor(options: ViewProps) {
super(options);
this.name = "OperatorExecutionButtonView";
}
static fromJSON(json) {
return new OperatorExecutionButtonView(json);
}
}


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 OperatorExecutionButtonView to VIEWS constant.

The new view class needs to be added to the VIEWS constant to support proper JSON deserialization through the viewFromJSON function.

 const VIEWS = {
   View,
   InferredView,
   Form,
   ReadOnlyView,
   // ... other views
   PillBadgeView,
   ModalView,
   ToastView,
+  OperatorExecutionButtonView,
 };

Committable suggestion skipped: line range outside the PR's diff.

/**
* Places where you can have your operator placement rendered.
*/
Expand Down
37 changes: 37 additions & 0 deletions fiftyone/operators/types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1370,6 +1370,43 @@ def to_json(self):
)


class OperatorExecutionButtonView(Button):
"""Represents an operator execution button in a :class:`View`.

Examples::

import fiftyone.operators.types as types

operatorButtonView = types.OperatorExecutionButtonView(
icon="expand_more",
operator="execute_custom_operator",
params={"key": "value"},
label="Execute",
description="Executes the specified operator",
)

inputs = types.Object()
inputs.view("operator_btn", operatorButtonView)

Args:
icon (str): an icon for the button. Defaults to "expand_more" if not provided.
label (str): a label for the button.
description (str): a description for the button.
title (str): a tooltip title for the button.
operator (str): the name of the operator to execute when the button is clicked.
params (dict): the parameters to pass to the operator.
prompt (str): a prompt for the operation.
disabled (bool): whether the button is disabled.
Comment on lines +1392 to +1399
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

Correct parameter types and defaults in the docstring

There are discrepancies between the docstring and the method implementation:

  • The prompt parameter is documented as (str) but should be (bool).
  • Default values mentioned in the docstring (e.g., icon defaults to "expand_more") are not reflected in the code.

Suggested fix:

Update the docstring to accurately reflect parameter types and default values.

 Args:
     icon (str, optional): an icon for the button. Defaults to "expand_more".
     label (str, optional): a label for the button.
     description (str, optional): a description for the button.
     title (str, optional): a tooltip title for the button.
     operator (str): the name of the operator to execute when the button is clicked.
     params (dict, optional): the parameters to pass to the operator.
-    prompt (str): a prompt for the operation.
+    prompt (bool, optional): whether to prompt the user before executing the operator.
     disabled (bool, optional): whether the button is disabled.

Additionally, align the default values in the code with those specified in the docstring.

Committable suggestion skipped: line range outside the PR's diff.

"""

def __init__(self, **kwargs):
if "operator" not in kwargs or not isinstance(kwargs["operator"], str):
raise ValueError(
"The 'operator' parameter of type str is required."
)
super().__init__(**kwargs)
Comment on lines +1402 to +1407
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

Define explicit __init__ parameters for clarity and type safety

Explicitly defining parameters in the __init__ method enhances readability and allows for better type checking. It makes the expected inputs clear to users and improves maintainability. Consider replacing **kwargs with explicit parameters.

Proposed change:

-def __init__(self, **kwargs):
+def __init__(
+    self,
+    operator: str,
+    icon: str = "expand_more",
+    label: Optional[str] = None,
+    description: Optional[str] = None,
+    title: Optional[str] = None,
+    params: Optional[dict] = None,
+    prompt: bool = False,
+    disabled: bool = False,
+    **kwargs,
+):
     if not isinstance(operator, str):
         raise ValueError("The 'operator' parameter of type str is required.")
-    super().__init__(**kwargs)
+    super().__init__(
+        operator=operator,
+        icon=icon,
+        label=label,
+        description=description,
+        title=title,
+        params=params,
+        prompt=prompt,
+        disabled=disabled,
+        **kwargs,
+    )
📝 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
def __init__(self, **kwargs):
if "operator" not in kwargs or not isinstance(kwargs["operator"], str):
raise ValueError(
"The 'operator' parameter of type str is required."
)
super().__init__(**kwargs)
def __init__(
self,
operator: str,
icon: str = "expand_more",
label: Optional[str] = None,
description: Optional[str] = None,
title: Optional[str] = None,
params: Optional[dict] = None,
prompt: bool = False,
disabled: bool = False,
**kwargs,
):
if not isinstance(operator, str):
raise ValueError("The 'operator' parameter of type str is required.")
super().__init__(
operator=operator,
icon=icon,
label=label,
description=description,
title=title,
params=params,
prompt=prompt,
disabled=disabled,
**kwargs,
)



class OneOfView(View):
"""Displays one of the given :class:`View` instances.

Expand Down
Loading