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 ToastView to Python Panels #4968

Merged
merged 6 commits into from
Oct 23, 2024
Merged

Add ToastView to Python Panels #4968

merged 6 commits into from
Oct 23, 2024

Conversation

Br2850
Copy link
Contributor

@Br2850 Br2850 commented Oct 23, 2024

What changes are proposed in this pull request?

  • Refactor @minhtuev 's Toast component to include ability to render ToastView in Python Panel context
  • Added ToastView to Python Panel
Screen.Recording.2024-10-23.at.2.01.27.AM.mov

How is this patch tested? If it is not, please explain why.

Tested locally in Python Panel context

Release Notes

Is this a user-facing change that should be mentioned in the release notes?

  • No. You can skip the rest of this section.
  • Yes. Give a description of this change to be included in the release
    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?

  • App: FiftyOne application changes
  • Build: Build and test infrastructure changes
  • Core: Core fiftyone Python library changes
  • Documentation: FiftyOne documentation changes
  • Other

Summary by CodeRabbit

  • New Features

    • Introduced a customizable Toast component with optional properties for layout and button functionalities.
    • Added a new ToastView component for displaying toast messages with customizable duration and layout.
    • Implemented a new ToastView class for enhanced integration within the application.
  • Bug Fixes

    • Enhanced rendering logic to ensure buttons are only displayed when provided.
  • Documentation

    • Updated exports to include the new ToastView component for easier access within the application.

Copy link
Contributor

coderabbitai bot commented Oct 23, 2024

Walkthrough

The changes in this pull request involve modifications to the Toast component and the introduction of a new ToastView component. The ToastProps interface has been updated to make certain properties optional and to add a new layout property for customizable styling. A new ToastView component has been created to utilize the Toast component, passing relevant props for rendering. Additionally, a new ToastView class has been added to both the TypeScript and Python codebases, ensuring consistent functionality across platforms.

Changes

File Change Summary
app/packages/components/src/components/Toast/Toast.tsx Updated ToastProps interface to make primary and secondary optional; added layout property for customization. Updated Toast component to destructure layout and apply dynamic styling.
app/packages/core/src/plugins/SchemaIO/components/ToastView.tsx Introduced new ToastView component that uses Toast, extracting and passing message, duration, and layout.
app/packages/core/src/plugins/SchemaIO/components/index.ts Added export statement for the new ToastView component.
app/packages/operators/src/types.ts Added new ToastView class extending View, with a constructor for options and a static fromJSON method.
fiftyone/operators/types.py Added new ToastView class for displaying toast messages with customizable parameters.

Possibly related PRs

  • Toast component #4955: The introduction of the Toast component is directly related to the main PR, as both involve modifications to the Toast component in Toast.tsx, enhancing its functionality and usability.
  • Toast component refactor #4957: This PR refactors the Toast component, which is the same component being modified in the main PR. The changes in both PRs focus on improving the Toast component's structure and functionality.

Suggested reviewers

  • imanjra
  • ritch

Poem

In the garden of code, a Toast does bloom,
With colors and layouts, it brightens the room.
A new view hops in, bringing joy to the day,
Customizable messages, in a delightful way!
So let’s raise a cheer, for the changes we see,
A toast to our efforts, from the rabbit, with glee! 🐇✨


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 8

🧹 Outside diff range and nitpick comments (4)
app/packages/core/src/plugins/SchemaIO/components/ToastView.tsx (2)

1-10: Add JSDoc documentation for the component.

The component lacks documentation explaining its purpose, props, and usage examples.

Add documentation at the top of the component:

+/**
+ * ToastView component that renders a Toast message based on schema configuration.
+ * 
+ * @component
+ * @example
+ * ```tsx
+ * const schema = {
+ *   view: {
+ *     message: "Operation successful",
+ *     duration: 3000,
+ *     layout: { position: "top-right" }
+ *   }
+ * };
+ * 
+ * <ToastView schema={schema} />
+ * ```
+ */
 export default function ToastView(props) {

6-7: Consider using nullish coalescing for view default.

Using the nullish coalescing operator (??) would be more appropriate than the logical OR operator (=) for defaulting the view object.

-  const { view = {} } = schema;
+  const { view } = schema ?? {};
+  const viewData = view ?? {};
-  const { message, duration, layout } = view;
+  const { message, duration, layout } = viewData;
fiftyone/operators/types.py (2)

1817-1837: Enhance documentation with layout parameter structure.

The documentation should clearly specify the structure of the layout parameter.

Add this to the Args section:

     Args:
         message: the message to display
         duration (None): the duration to stay on screen in milliseconds
-        layout (None): the layout of the toast
+        layout (None): the layout of the toast. A dictionary with the following structure:
+            {
+                "vertical": str,   # "top" or "bottom"
+                "horizontal": str, # "left", "center", or "right"
+                "top": str,       # CSS distance from top (e.g., "200px")
+            }

1821-1829: Improve schema example formatting.

The schema example could be more readable with proper indentation.

Apply this formatting:

     schema = {
-            "message": "Test",
-            "duration": 30000,
-            "layout": {
-                "vertical": "top",
-                "horizontal": "center",
-                "top": "200px"
-            },
+        "message": "Test",
+        "duration": 30000,
+        "layout": {
+            "vertical": "top",
+            "horizontal": "center",
+            "top": "200px"
+        }
     }
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 866c8c6 and 5771111.

📒 Files selected for processing (5)
  • app/packages/components/src/components/Toast/Toast.tsx (2 hunks)
  • app/packages/core/src/plugins/SchemaIO/components/ToastView.tsx (1 hunks)
  • app/packages/core/src/plugins/SchemaIO/components/index.ts (1 hunks)
  • app/packages/operators/src/types.ts (2 hunks)
  • fiftyone/operators/types.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
app/packages/components/src/components/Toast/Toast.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/ToastView.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/index.ts (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/operators/src/types.ts (1)

Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.

🪛 Biome
app/packages/components/src/components/Toast/Toast.tsx

[error] 68-68: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


[error] 69-69: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🔇 Additional comments (5)
app/packages/core/src/plugins/SchemaIO/components/ToastView.tsx (1)

1-2: 🛠️ Refactor suggestion

Consider adding prop-types for runtime validation.

While TypeScript provides compile-time type checking, adding PropTypes would provide runtime validation for better debugging.

Add PropTypes for runtime validation:

 import React from "react";
 import { Toast } from "@fiftyone/components";
+import PropTypes from 'prop-types';

 // ... component implementation ...

+ToastView.propTypes = {
+  schema: PropTypes.shape({
+    view: PropTypes.shape({
+      message: PropTypes.string,
+      duration: PropTypes.number,
+      layout: PropTypes.object
+    })
+  }).isRequired
+};

Likely invalid or redundant comment.

app/packages/core/src/plugins/SchemaIO/components/index.ts (2)

53-53: LGTM! Export statement follows established conventions.

The new ToastView export maintains consistency with the existing pattern and preserves alphabetical ordering.


53-53: Verify ToastView file existence.

Let's ensure the referenced ToastView.tsx file exists in the components directory.

✅ Verification successful

ToastView component file exists as expected

The verification confirms that ToastView.tsx exists at the expected location app/packages/core/src/plugins/SchemaIO/components/ToastView.tsx, matching the export statement in the index file.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the existence of ToastView.tsx in the components directory
# Expected: File should exist in the same directory as index.ts

fd --type f "ToastView.tsx" "app/packages/core/src/plugins/SchemaIO/components"

Length of output: 141

app/packages/operators/src/types.ts (2)

1199-1211: LGTM! The ToastView implementation follows the established patterns.

The class is well-structured with proper TypeScript types and includes the necessary static fromJSON method for serialization.


1284-1284: LGTM! ToastView is properly registered in the VIEWS constant.

The addition maintains consistency with other view registrations.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (2)
app/packages/components/src/components/Toast/Toast.tsx (2)

35-36: Extract magic number into a named constant.

The default height of 5 is a magic number. Consider extracting it into a named constant for better maintainability and documentation.

+const DEFAULT_SNACKBAR_HEIGHT = 5;
+
 const snackbarStyle = {
-  height: layout?.height || 5,
+  height: layout?.height || DEFAULT_SNACKBAR_HEIGHT,

Line range hint 62-65: Add proper TypeScript types to event handler.

The handleClose function lacks proper TypeScript types for its parameters.

- const handleClose = (event, reason) => {
+ const handleClose = (
+   event: React.SyntheticEvent | Event,
+   reason: string
+ ) => {
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 7f18f65 and 4ee5efc.

📒 Files selected for processing (1)
  • app/packages/components/src/components/Toast/Toast.tsx (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/components/src/components/Toast/Toast.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.

🪛 Biome
app/packages/components/src/components/Toast/Toast.tsx

[error] 70-70: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


[error] 71-71: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

height: layout?.height || 5,
...(layout?.top && {
top: {
xs: layout.top,
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these values supposed to be the same? Just checking :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this will conditionally add top or bottom only if they exist coming in from the Python Panel context

Copy link
Contributor

@minhtuev minhtuev left a comment

Choose a reason for hiding this comment

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

Nice, LGTM! Maybe someone from @voxel51/frontend-devs can have a 2nd look :)

@Br2850 Br2850 merged commit b1d407e into develop Oct 23, 2024
13 checks passed
@Br2850 Br2850 deleted the patch/toast-py-panels branch October 23, 2024 17:30
This was referenced Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants