widgetti / solara

A Pure Python, React-style Framework for Scaling Your Jupyter and Web Apps
https://solara.dev
MIT License
1.62k stars 105 forks source link

Feat: add switch widget #185

Closed lp9052 closed 10 months ago

lp9052 commented 10 months ago

add switch widget

see #174

lp9052 commented 10 months ago

Do we have permission to rerun failed test?

maartenbreddels commented 10 months ago

Thank you 🙏

Apparently, you need to have write access for the repo to do that. I'm happy to restart it though

lp9052 commented 10 months ago

Thanks Maarten.

I guess checkbox is one of the earlier piece that doesn't have all the new styles. All the enhancements make sense to me. I shall update checkbox too. I'll start a new PR for checkbox.

lp9052 commented 10 months ago

take a look.

one small concern, we are passing [] as default parameter, it's mutable.

should we do the following:

    children: Optional[list] = None,
    classes: Optional[List[str]] = None,
maartenbreddels commented 10 months ago

I guess checkbox is one of the earlier piece that doesn't have all the new styles. All the enhancements make sense to me. I shall update checkbox too. I'll start a new PR for checkbox.

indeed, and that would be great!

one small concern, we are passing [] as default parameter, it's mutable.

Yes, that's also something I usually frown upon... we are using that in many places because it also keeps the typing simpler so lets stick to it for now and if we change it, we'll do it throughout the whole codebase.

lp9052 commented 10 months ago

hmmm, the testing suit seems somewhat unstable.

also I want to open up an issue about unit test doesn't all pass out of the box.

it requires some other packages as solar enterprise, etc.

maartenbreddels commented 10 months ago

Hmm, it's been a while since that test failed. Before we saw that one fail more often, see https://github.com/widgetti/solara/issues/131 for some comments/clues. The integration test also has 1 flakey test.

lp9052 commented 10 months ago

lmk if you have more comments else we can merge this.

I have one more PR coming up.

Thanks

lp9052 commented 10 months ago

Hmm, it's been a while since that test failed. Before we saw that one fail more often, see #131 for some comments/clues. The integration test also has 1 flakey test.

it's a quite interesting issue. I always like to see coverage info. And in my opinion 100% coverage is a must if we want to push this project further and accept more contributors

maartenbreddels commented 10 months ago

I agree. A possible option might be to explicitly set intrusive_cancel in https://solara.dev/api/use_thread and only test that intrusive_cancel feature separately. Still, I find it really suspicious/odd that even after 10 seconds it does not finish the thread execution. I've stared at this failure many times and thought about it many times, but it might need a fresh pair of eyes.

maartenbreddels commented 10 months ago

There are 2 few missing I think TODO:

I'm happy to pick this up if this is too much to ask. But this would also make this PR a nice template for new components.

lp9052 commented 10 months ago

There are 2 few missing I think

TODO:

  • [x] Add to solara/website/pages/api/init.py

  • [x] Add a gif to solara/website/public/api/

I'm happy to pick this up if this is too much to ask. But this would also make this PR a nice template for new components.

Give me a day or two. I might run into problem for gif. But I'll let you know. Thanks.

lp9052 commented 10 months ago

Added these two pieces per suggestion.

lp9052 commented 10 months ago

@maartenbreddels

maartenbreddels commented 10 months ago

Thank you! 🙏