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

feat: add on click callback #1

Merged

Conversation

tilman151
Copy link
Contributor

Hey, I recently needed some callbacks from Plotly in FastHTML and I figured your package might be the right place to put it.

I included the JS code in the plotly_headers and render a call to it when registering a callback. You can have a look at the example app to see it in action. If what I wrote is acceptable for you I'd add some tests to it before merging. Would adding playwright as a dev dependency be okay for you then? I'd like to actually do the click during an end2end test.

@CarloLepelaars
Copy link
Owner

Hi @tilman151, thank you so much for the contribution!

The implementation looks great. Adding playwright as a dev dependency is ok. Are you familiar with uv (uv add playwright)? Also feel free to set the version to 0.2.1.

If you could add some tests and they pass I think we are ready to merge.

@tilman151
Copy link
Contributor Author

Yeah, I'm familiar with uv. Will try to do it tomorrow.

@tilman151
Copy link
Contributor Author

tilman151 commented Dec 5, 2024

I added the tests to the CI in a separate job because setting up playwright is a time-consuming operation.

On another note: you set up your dev dependencies as an extra. Typically they are added as a dependency group in uv so that they are automatically installed when calling uv sync but not included in the PyPI build. Up to you.

@CarloLepelaars
Copy link
Owner

CarloLepelaars commented Dec 5, 2024

Good catch on the dev dependencies! Feel free to add them as a dependency group instead of extras.

Something failing with playwright install?

@tilman151
Copy link
Contributor Author

Ah, yeah. Forgot to deactivate the e2e tests in the test job and forgot to install the extra deps in the e2e job. Let me fix that.

@tilman151
Copy link
Contributor Author

Should run now, hopefully.

@CarloLepelaars CarloLepelaars merged commit 81e13f7 into CarloLepelaars:main Dec 5, 2024
7 checks passed
@CarloLepelaars
Copy link
Owner

Amazing, thank you! Will release it now for v0.2.1

@CarloLepelaars
Copy link
Owner

Released! 😄

https://pypi.org/project/fh-plotly

@tilman151
Copy link
Contributor Author

If you want, I can also have a look at the selection callback. Shouldn't be that hard, now that the scaffolding is there.

@CarloLepelaars
Copy link
Owner

If you want, I can also have a look at the selection callback. Shouldn't be that hard, now that the scaffolding is there.

Great idea! Additional PRs are definitely appreciated.

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