Add a VerticalSlider class based on the Slider class

[ermarch]

This adds a vertical slider class inheriting the original horizontal slider class.

[wjakob]

Hi @ermarch -- I think it would be nicer to make vertical/horizontal an attribute of the Slider class (preferably, with the minimal amount of changes/code duplication) rather than having an entirely new class.

[ermarch]

On 5/24/19 5:30 PM, Wenzel Jakob wrote:

Hi @ermarch https://github.com/ermarch -- I think it would be nicer to make vertical/horizontal an attribute of the Slider class (preferably, with the minimal amount of changes/code duplication) rather than having an entirely new class.

I've been thinking about that too, but I see no real reason to adjust the direction at run-time and I think doing it this way makes the code much cleaner.

If you still want it that way I could change the code though.

Erik

[wjakob]

Hi Erik,

it's generally a good rule to avoid adding extra API as much as possible. When a new class just presents a different way of rendering the same concept, then it's preferable to expose it in this way, using an extra attribute. (Whether that can be used to switch at runtime is another question and not the primary motivator.)

Best, Wenzel

[ermarch]

Okay, but that would mean a number of if statements to distinguish between both modes. I'll change the code and send a new pull request.

Erik

[wjakob]

For readability, it would be best to minimize the number of if-statements by having a set of variables storing sizes, positions, etc. that are assigned using the ternary operator. You'll want to minimize the amount of redundancy, which requires some planning (instead of replacing every line by an if statement)

[ermarch]

Well, that's the problem, you need to change the X and Y values for size, color and mouse events. It's not as easy as you might think. I've tried, and it's not.

It's much leaner to just redefine the mouse and draw functions.

Erik