-
Notifications
You must be signed in to change notification settings - Fork 141
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
Feat: add switch widget #185
Conversation
Do we have permission to rerun failed test? |
Thank you 🙏 Apparently, you need to have write access for the repo to do that. I'm happy to restart it though |
There was a problem hiding this 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 👍
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. |
take a look. one small concern, we are passing should we do the following:
|
indeed, and that would be great!
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. |
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. |
Hmm, it's been a while since that test failed. Before we saw that one fail more often, see #131 for some comments/clues. |
lmk if you have more comments else we can merge this. I have one more PR coming up. Thanks |
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 |
I agree. A possible option might be to explicitly set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
eeaae64
to
9dd7e52
Compare
There are 2 few missing I think
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. |
Added these two pieces per suggestion. |
1b55c00
to
a471cbd
Compare
- typing style
add switch gif
a471cbd
to
028279a
Compare
Thank you! 🙏 |
add switch widget
see #174