-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
Expand on slider_demo example #19264
Conversation
Co-Authored-By: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
@timhoffm what do you think of removing the I also added named arguments to the creation of the I resolved the above reviews after addressing them to remove the clutter. But was that poor form/rude? Is that something I should leave to you given that you opened them? |
👍
No, that's exactly how it should be. Generally, the author should make all changes and also adapt the PR to comments of the reviewers. In rare cases, reviewers may directly want to add commits (e.g. if there's some complex change that's difficult to explain or to push an inactive PR towards completion). But this will be announced explicitly. |
I like where this is going. My only remaining gripes with this example are:
To be specific I meant the act of pressing this button: |
Looking at the other examples they have a lot of repeated code. It feels like Sliders (or maybe widgets more broadly) deserve their own dedicated (short) long-form tutorial. In particular things like |
I don't know that we have a set etiquette for "resolve changes" but as a PR reviewer I prefer the author not hit that button unless it has been a while. The reviewer can hit it if they think it has been resolved. Otherwise the conversation gets hidden and as a reviewer I either have to un-hide it or I forget about it. |
as @jklymak says we don't have a strict policy on that. Personally, I think this is a good policy:
This saves me from having to look again on straight forward changes. |
You can leave this to #19265
👍 Do that for the frequency. The default is not a particularly special value anyway. |
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.
I like it! Should we go further and put the pro: demonstrates the format string |
Keep it simple and leave it with the label. |
Co-Authored-By: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Should I squash this all into one commit? |
It should be one commit to not unnecessarily blow up the history. But we can also "Squash and merge" via the GitHub UI. That's good enough. |
PR Summary
I tried to address all the things I remember finding frustrating when I first figuring out widgets
RadioButtons
sfreq
->freq_slider
)PR Checklist
pytest
passes).flake8
on changed files to check).flake8-docstrings
and runflake8 --docstring-convention=all
).doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).