Make ImageView clickable/linkable in Python Panels #4996
Conversation
WalkthroughThe pull request introduces enhancements to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
fiftyone/operators/types.py (1)
1780-1788: Consider enhancing the example to demonstrate both link and operator usage.The example shows basic usage but could be more comprehensive by demonstrating both ways to make an image interactive:
- Opening a URL via
href- Calling an operator with parameters
Consider updating the example to show both cases:
def execute(): return {"image": "https://voxel51.com/your/image.png"} def resolve_output(self, ctx): # Example 1: Image with a link schema1 = { "height": "100px", "width": "100px", "alt": "Click to visit Voxel51", "href": "https://voxel51.com", } # Example 2: Image with an operator schema2 = { "height": "100px", "width": "100px", "alt": "Click to process image", "operator": self.do_something, "prompt": True, # Show confirmation dialog "params": {"foo": "bar"}, }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/packages/core/src/plugins/SchemaIO/components/ImageView.tsx (2 hunks)
- fiftyone/operators/types.py (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/core/src/plugins/SchemaIO/components/ImageView.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (3)
app/packages/core/src/plugins/SchemaIO/components/ImageView.tsx (2)
5-6: LGTM: Hook imports follow React conventionsThe new hook imports are properly organized and follow React naming conventions.
21-22: LGTM: Proper hook usageHooks are correctly implemented at the top level of the component.
fiftyone/operators/types.py (1)
1798-1805: LGTM! Parameters are well-documented and align with requirements.The new parameters (
height,width,alt,href,operator,prompt,params) are properly documented and provide the necessary functionality to make images clickable/linkable as specified in the PR objectives.
| const { | ||
| height, | ||
| width, | ||
| alt, | ||
| href, | ||
| operator, | ||
| prompt = false, | ||
| params, | ||
| } = schema?.view || {}; |
There was a problem hiding this comment.
Add TypeScript type definitions for props and schema
The component lacks proper TypeScript types for its props and schema structure. This could lead to runtime errors if invalid props are passed.
Consider adding type definitions:
interface ImageViewProps {
schema: {
view?: {
height?: number;
width?: number;
alt?: string;
href?: string;
operator?: string;
prompt?: boolean;
params?: Record<string, unknown>;
};
default?: string;
};
data?: string;
}
app/packages/core/src/plugins/SchemaIO/components/ImageView.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/packages/core/src/plugins/SchemaIO/components/ImageView.tsx (1)
49-49: Update cursor style to indicate interactivity when'operator'is definedCurrently, the cursor changes to a pointer only when
hrefis defined. However, the image is also interactive when anoperatoris specified. To improve user experience, update the cursor style to indicate interactivity whenever eitherhreforoperatoris defined.Apply this diff to adjust the cursor styling:
onClick={onClick} - style={{ cursor: href ? "pointer" : "default" }} // Change cursor based on href + style={{ cursor: (href || operator) ? "pointer" : "default" }} // Change cursor based on href or operator {...getComponentProps(props, "image")}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/packages/core/src/plugins/SchemaIO/components/ImageView.tsx (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/core/src/plugins/SchemaIO/components/ImageView.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/plugins/SchemaIO/components/ImageView.tsx
Outdated
Show resolved
Hide resolved
app/packages/core/src/plugins/SchemaIO/components/ImageView.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/packages/core/src/plugins/SchemaIO/components/ImageView.tsx (2 hunks)
- fiftyone/operators/types.py (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/core/src/plugins/SchemaIO/components/ImageView.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (4)
app/packages/core/src/plugins/SchemaIO/components/ImageView.tsx (2)
11-20: LGTM! Clean implementation of new parameters
The destructuring is well-organized with sensible defaults for prompt and cursor. The new parameters align well with the PR objectives for making images clickable and linkable.
59-60: LGTM! Clean implementation of click handling and cursor feedback
The cursor styling provides good visual feedback for clickable images, and the implementation is clean and straightforward.
fiftyone/operators/types.py (2)
312-370: LGTM! Well-implemented image configuration method.
The new img method provides a clean interface for configuring clickable/linkable images in Python Panels. The implementation is thorough and well-documented.
Line range hint 1840-1865: LGTM! Clear and comprehensive documentation.
The updated ImageView docstring accurately describes all new parameters and provides helpful examples for both URL links and operator execution.
app/packages/core/src/plugins/SchemaIO/components/ImageView.tsx
Outdated
Show resolved
Hide resolved
app/packages/core/src/plugins/SchemaIO/components/ImageView.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/packages/core/src/plugins/SchemaIO/components/ImageView.tsx (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/core/src/plugins/SchemaIO/components/ImageView.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
| if (result?.error) { | ||
| console.log(result?.error); | ||
| console.log(result?.errorMessage); |
There was a problem hiding this comment.
Enhance error handling with Toast notifications
Currently, errors are only logged to the console. Users won't be aware if an operation fails.
if (result?.error) {
- console.log(result?.error);
- console.log(result?.errorMessage);
+ import { toast } from "@fiftyone/spaces";
+ toast.error(`Operation failed: ${result?.errorMessage || 'Unknown error'}`);
+ console.error('Operation failed:', { error: result?.error, message: result?.errorMessage });Committable suggestion was skipped due to low confidence.
What changes are proposed in this pull request?
Give images the ability to:
.img()built in method to allow Python Panels a shortcut to define imagesUpdated docstrings and examples to correctly identify how to use
.img()andImageViewHow is this patch tested? If it is not, please explain why.
See screen recording below w/ code
Screen.Recording.2024-10-28.at.11.35.19.AM.mov
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
Summary by CodeRabbit
New Features
ImageViewcomponent with additional properties for improved interactivity.imgmethod to define image properties for display.Documentation
ImageViewclass to include new parameters, improving clarity for developers.