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 disable keyword to select and select multiple widgets #173

Conversation

lp9052
Copy link
Contributor

@lp9052 lp9052 commented Jun 23, 2023

add disabled keyword to select widgets
see #144

@lp9052
Copy link
Contributor Author

lp9052 commented Jun 23, 2023

first PR. Want to use this to familiarize with the process.

Let me know if there are specific rules for this project.

I followed instruction from development page.

Once this merged, I'll be able to contribute more.

Happy Friday.

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.

Nice, thank you!

Just a few suggestions. Also in components_test.py we also have a test_select. I think we can remove that one, this one is better.

solara/components/select.py Outdated Show resolved Hide resolved
tests/unit/select_test.py Outdated Show resolved Hide resolved
tests/unit/select_test.py Outdated Show resolved Hide resolved
@lp9052
Copy link
Contributor Author

lp9052 commented Jun 23, 2023

Thanks Maarten!

I'll remove the test in component test.

And is there any specific reason we prefer Test() over under_test() or test()? As snakecase is the preferred convention. Pep8 will report otherwise.

@lp9052
Copy link
Contributor Author

lp9052 commented Jun 23, 2023

urrr. I don't see components_test.py. Can you point out to me where is it?

@maartenbreddels
Copy link
Contributor

I see it is never added to git 🙃
But we use the CamelCase because components are special functions, they are lazy, get re-executed, and contain state, which makes them behave more like classes.

@lp9052
Copy link
Contributor Author

lp9052 commented Jun 23, 2023

just merged changes and please take a look

@maartenbreddels maartenbreddels merged commit 9cac720 into widgetti:master Jun 23, 2023
21 checks passed
@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