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 switch widget #185

Merged
merged 4 commits into from
Jul 20, 2023

Conversation

lp9052
Copy link
Contributor

@lp9052 lp9052 commented Jun 30, 2023

add switch widget

see #174

@lp9052 lp9052 changed the title Feat: add switch widget lp9052 Feat: add switch widget Jun 30, 2023
@lp9052
Copy link
Contributor Author

lp9052 commented Jun 30, 2023

Do we have permission to rerun failed test?

@maartenbreddels
Copy link
Contributor

Thank you 🙏

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

Copy link
Contributor

@maartenbreddels maartenbreddels left a comment

Choose a reason for hiding this comment

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

Just a few minor points that would make this PR even more perfect 👍

solara/components/switch.py Outdated Show resolved Hide resolved
solara/components/switch.py Outdated Show resolved Hide resolved
@lp9052
Copy link
Contributor Author

lp9052 commented Jun 30, 2023

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
Copy link
Contributor Author

lp9052 commented Jun 30, 2023

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
Copy link
Contributor

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
Copy link
Contributor Author

lp9052 commented Jun 30, 2023

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
Copy link
Contributor

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.

@lp9052
Copy link
Contributor Author

lp9052 commented Jun 30, 2023

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

I have one more PR coming up.

Thanks

@lp9052
Copy link
Contributor Author

lp9052 commented Jun 30, 2023

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
Copy link
Contributor

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.

Copy link
Contributor

@maartenbreddels maartenbreddels left a comment

Choose a reason for hiding this comment

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

💯

@maartenbreddels
Copy link
Contributor

maartenbreddels commented Jul 2, 2023

There are 2 few missing I think
TODO:

  • Add to solara/website/pages/api/init.py
  • 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.

@lp9052
Copy link
Contributor Author

lp9052 commented Jul 2, 2023

There are 2 few missing I think

TODO:

  • Add to solara/website/pages/api/init.py

  • 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
Copy link
Contributor Author

lp9052 commented Jul 11, 2023

Added these two pieces per suggestion.

@lp9052
Copy link
Contributor Author

lp9052 commented Jul 19, 2023

@maartenbreddels

@maartenbreddels maartenbreddels merged commit cc7e0cd into widgetti:master Jul 20, 2023
@maartenbreddels
Copy link
Contributor

Thank you! 🙏

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