Rendered version here:https://51664-1385122-gh.circle-artifacts.com/0/doc/build/html/gallery/widgets/slider_demo.html#sphx-glr-gallery-widgets-slider-demo-py

@timhoffm what do you think of removing thevalstep from this example? It doesn't really get much space for an explanation here, and may mess up people who copy paste the sliders directly from the example. Instead I propose just mentioning it in the description and linking to the standalone snapping example.

I also added named arguments to the creation of theSliders as unnamed arguments can be a bit mystifying.

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?

@timhoffm what do you think of removing thevalstep from this example? It doesn't really get much space for an explanation here, and may mess up people who copy paste the sliders directly from the example. Instead I propose just mentioning it in the description and linking to the standalone snapping example.

👍

I resolved the above reviews after addressing them to remove the clutter. But was that poor form/rude?

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:

1. I don't like howlightgoldenrodyellow looks, but I understand why it's handy to demonstrate this
   • This will be affected byChange styling of slider widgets #19265 so may as well leave it here
2. There's no clear demonstration of how to remove the red line markingvalinit
   • Thoughts on settinginit_color='none' for at least one of the sliders?

I resolved the above reviews after addressing them to remove the clutter. But was that poor form/rude?
No, that's exactly how it should be.

To be specific I meant the act of pressing this button:

me pushing the changes makes sense, otherwise you'd be truly overwhelmed I imagine.

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 likevalstep feel a bit weird to devote an entire example to.

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.

To be specific I meant the act of pressing this button:

as@jklymak says we don't have a strict policy on that. Personally, I think this is a good policy:

• If the author is exactly implementing the proposed changes, it's fine to mark the conversation as resolved. I trust the author's judgement in that an agreement is achieved and implemented (maybe double-ckeck for first-time contributors or really complex changes).
• If there's still some discussion going on, or if you chose a solution different from the proposed change, leave the comments open.

This saves me from having to look again on straight forward changes.

1. I don't like howlightgoldenrodyellow looks, but I understand why it's handy to demonstrate this
   • This will be affected byChange styling of slider widgets #19265 so may as well leave it here

You can leave this to#19265

2. There's no clear demonstration of how to remove the red line markingvalinit
   • Thoughts on settinginit_color='none' for at least one of the sliders?

👍 Do that for the frequency. The default is not a particularly special value anyway.

What do you think about labeling frequency and the x-axis with units Hz and s like this:

I like it! Should we go further and put the[Hz] next to the number by using the format string?

pro: demonstrates the format string
con: may end up being confusing for the copy-pasters out there. The natural question for them will be what to replace it with and they may end up manually specifying a format string which can be difficult.

Keep it simple and leave it with the label.

Should I squash this all into one commit?

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.