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

Python Panels #4528

Open
wants to merge 144 commits into
base: develop
Choose a base branch
from
Open

Python Panels #4528

wants to merge 144 commits into from

Conversation

ritch
Copy link
Contributor

@ritch ritch commented Jun 24, 2024

Python Panels

const handleEvent =
(event) =>
(...args) => {
console.log(`Video event: ${event}`, ...args);
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this needs to be wired up or removed

Copy link
Contributor

@imanjra imanjra left a comment

Choose a reason for hiding this comment

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

We can continue to test and address comment as a follow-up. Good to check this in early as it's a large PR. LGTM 🚀

Copy link
Contributor

@benjaminpkane benjaminpkane left a comment

Choose a reason for hiding this comment

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

Can we please have passing e2e before merging? 🙏

@@ -46,6 +46,7 @@
"@textea/json-viewer": "^3.4.1",
"classnames": "^2.3.1",
"framer-motion": "^6.2.7",
"material-icons": "^1.13.12",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we bringing it in when we already have @mui/icons-material? This will cause significant bloating otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is font-based and it's to support dynamic icon by name. In a separate PR, we should probably replace all the icons from @mui/icons-material to this instead. It will reduce build size/time

Copy link
Contributor

@benjaminpkane benjaminpkane left a comment

Choose a reason for hiding this comment

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

LGTM. Will update e2e screenshots in #4281

Note, documentation builds are failing 🤔

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.

None yet

4 participants