Show slider range #454

cchan987 · 2015-06-30T16:08:14Z

Showing the max and min slider values adjacent to the slider
Addresses #123 , #464

…range

jgosmann · 2015-06-30T16:09:15Z

nengo_gui/static/slider.js

@@ -297,7 +349,10 @@ Nengo.Slider.prototype.set_range = function() {
            for (var i in self.sliders) {
                self.sliders[i].set_range(min, max);
            }
+            console.log('here')


Remove debug output?

jgosmann · 2015-06-30T16:56:47Z

The labels can randomly pop up and fade out again during resizing.
It might look better to have the labels closer to the vertical bar.
I like the fade out animation, but it is strange the the hover box instantly disappears.

jgosmann · 2015-06-30T17:03:21Z

nengo_gui/static/slider.js

@@ -145,6 +192,8 @@ Nengo.Slider.prototype.on_resize = function(width, height) {
    this.group.style.height = height - this.ax_top - 2 * this.border_size;
    this.group.style.marginTop = this.ax_top;

+    this.bound_labels.style.height = height - (this.ax_top - 10) - this.border_size ;


It is unclear what the relevance of magic number 10 is in this line. This should be a named constant/variable. I wonder whether this height calculation is needed at all ...

It is unclear what the relevance of magic number 10 is in this line. This should be a named constant/variable. I wonder whether this height calculation is needed at all ...

The height calculation here is used to have the numbers move up and down to correspond with vertical resizing. Agreed, on assigning this 10 somewhere.

jgosmann · 2015-06-30T17:06:26Z

Note that I refactored the sliders. slider.js is only a container which contains multiple SliderControl instances. I think, the appropriate place to implement those range labels would be slidercontrol.js and not slider.js.

cchan987 · 2015-06-30T17:15:29Z

Note that I refactored the sliders. slider.js is only a container which contains multiple SliderControl instances. I think, the appropriate place to implement those range labels would be slidercontrol.js and not slider.js.

Hmm, I was thinking that even if we had 5 sliders lined up, we should only have the min max labelled once so as to prevent it from getting to cluttered.

cchan987 · 2015-06-30T17:18:17Z

I like the fade out animation, but it is strange the the hover box instantly disappears.

Hover box disappears? Can you explain this one a bit more?

jgosmann · 2015-06-30T17:21:45Z

Hmm, I was thinking that even if we had 5 sliders lined up, we should only have the min max labelled once so as to prevent it from getting to cluttered.

True, as long as all sliders are required to have the same range. But it could still be a more flexible implementation to implement it in slidercontrol.js and only display the range for the first SliderControl (either by having a flag in SliderControl whether to display the range on hover or having the mouse event in Slider and toggle the display of the range for the first slider in that event handler).

Hover box disappears? Can you explain this one a bit more?

If you move the mouse over the slider, you get this gray box around it. If you move the cursor away, that box disappears instantly whereas the range fades out.

tcstewar · 2015-06-30T17:25:00Z

Hmm, I was thinking that even if we had 5 sliders lined up, we should only have the min max labelled once so as to prevent it from getting to cluttered.

I agree that there should only be one min/max no matter how many sliders there are in the one Component.

cchan987 · 2015-07-06T15:11:51Z

For positioning, Would it be preferred to have the range labels follow the first slider guideline left and right while the slider is being resized horizontally, or have the labels to always be on the left hand side of the slider component (current implementation)?

I am in favour the latter option so the labels act as a sort of anchor while resizing horizontally, also moving more items makes resizing a more visually 'busy' operation. Keeping things simple this way.

cchan987 · 2015-07-06T15:13:11Z

The labels can randomly pop up and fade out again during resizing.

this is also the case with the border of all components currently. This should probably be addressed in it's own PR if we want to rectify this.

tcstewar · 2015-07-06T15:33:03Z

this is also the case with the border of all components currently. This should probably be addressed in it's own PR if we want to rectify this.

If you want to do that as a separate PR, sure, but it's important enough to this PR that I'm pretty sure it'll have to be addressed before this PR is accepted. (Basically, this PR is making that hover effect do a lot more than it used to, so it makes that resize problem much more obvious)

jgosmann · 2015-07-06T16:45:37Z

See issue #464

tcstewar · 2015-07-13T17:12:13Z

How do people feel about the look of these range indicators? I think the functionality is about right, but there's something wrong with the look to me. Perhaps the numbers could be shown in the same font as the numbers in the ranges shown on the graphs?

Also, how do people feel about the numbers being underneath the sliders themselves? Here's what it looks like now:

jgosmann · 2015-07-13T17:29:13Z

Perhaps the numbers could be shown in the same font as the numbers in the ranges shown on the graphs?

Would make sense to me.

I don't like that the numbers get covered. That looks like a bug.

cchan987 · 2015-07-13T17:50:22Z

By widening the component it can be made so that the range labels are no longer overlapped.
The alternative could be to have the labels in their own column to the left of the sliders, however I feel that makes the labels disconnected.

jgosmann · 2015-07-13T17:54:13Z

How about rotating the labels by 90 degress to use the whitespace inbetween sliders more effectively?

tcstewar · 2015-07-13T17:57:17Z

How about rotating the labels by 90 degress to use the whitespace inbetween sliders more effectively?

Definitely an option to try out.

I'd also try out actually using the same D3 Axis thing so that there's both the axis labels and a line with three tick marks on the far left of all the sliders.

tbekolay · 2015-07-13T20:40:08Z

three tick marks on the far left of all the sliders.

All the sliders or just the left one? I could see the tick marks being on all, but only the range labels on the far left one? Since they all share a range, having it repeated would be redundant / cluttered.

tcstewar · 2015-07-13T21:11:41Z

All the sliders or just the left one? I could see the tick marks being on all, but only the range labels on the far left one? Since they all share a range, having it repeated would be redundant / cluttered.

Sorry, that's me not being clear. I meant a single separate axis on the far left. So something like this, only with the numbers correct and everything lined up right:

jgosmann · 2015-07-13T21:32:48Z

I think that separate axis would look good. Can we get rid of the tick to the right at 0? Ant the extension to the bottom of the axis?

tcstewar · 2015-07-13T21:41:19Z

I think that separate axis would look good. Can we get rid of the tick to the right at 0? Ant the extension to the bottom of the axis?

Definitely. Those just didn't get removed because I was making a mockup in a paint program I wasn't familiar with. :)

cchan987 · 2015-07-29T15:56:13Z

How about rotating the labels by 90 degress to use the whitespace inbetween sliders more effectively?

Updated to demo this in 5be35ce , doesn't seem right to me. I will try out the axis idea next.

cchan987 · 2015-07-29T16:01:39Z

In 6795a12 I reduced the left margin so the slider guidelines don't cover it unless the component is being squeezed extremely narrow. This seems to be a clean simple fix.

cchan987 · 2015-07-29T16:05:01Z

I wonder if it's more convenient to quote commit IDs or should I set up different branches for testing ideas.

tcstewar · 2015-07-31T18:47:18Z

I wonder if it's more convenient to quote commit IDs or should I set up different branches for testing ideas.

Different branches is useaully preferred, I think -- that way it doesn't clutter up the history.

tcstewar · 2015-07-31T18:47:47Z

Updated to demo this [rotation] in 5be35ce , doesn't seem right to me. I will try out the axis idea next.

Hmm... yeah, I agree that the rotation doesn't seem to look right

tcstewar · 2015-07-31T18:48:57Z

In 6795a12 I reduced the left margin so the slider guidelines don't cover it unless the component is being squeezed extremely narrow. This seems to be a clean simple fix.

That's definitely an improvement. I still find it odd that it's in a completely different font and size from all our other range labels, though. Are you planning on trying the single axis suggestion?

…range

cchan987 · 2015-08-10T17:24:49Z

Are you planning on trying the single axis suggestion?

see #526

Seanny123 · 2015-08-15T00:45:49Z

LGTM. Is there any reason this still needs changes? Other than the fact that the commits need to be squashed together?

tcstewar · 2015-08-31T21:18:21Z

Is there any reason this still needs changes? Other than the fact that the commits need to be squashed together?

I much prefer the look of what's being attempted in #526 . As it stands, I'm not sure this PR is an improvement over what we currently have, and I think #526 with some tweaks would work quite well.

cchan987 added 3 commits June 29, 2015 15:02

initial commit

6439de2

Merge branch 'master' of github.com:nengo/nengo_gui into show_slider_…

8190db1

…range

removed margin

da0c71e

jgosmann reviewed Jun 30, 2015
View reviewed changes

cchan987 added 2 commits June 30, 2015 12:53

removed debug code

e12ca1e

simplified show/hide functions

885b470

jgosmann reviewed Jun 30, 2015
View reviewed changes

Seanny123 added the needs changes label Jul 1, 2015

eliminate fadeout, variablize height calc

92f3a7e

cchan987 added 2 commits July 6, 2015 14:38

range label improved, wip

694d917

Fixed broken value plots

b71af10

cchan987 added needs review and removed needs changes labels Jul 13, 2015

cchan987 added 2 commits July 29, 2015 11:30

merge with master

7d87b60

rotated labels

5be35ce

reduced left margin, looks better

6795a12

cchan987 added needs changes and removed needs review labels Jul 29, 2015

Merge branch 'master' of github.com:nengo/nengo_gui into show_slider_…

eba9345

…range

Seanny123 added reviewed and removed needs changes labels Aug 15, 2015

tcstewar removed the reviewed label Aug 31, 2015

tcstewar mentioned this pull request Jan 26, 2017

Show range on the sliders #123

Open

tcstewar force-pushed the master branch from dbbc7b1 to 08749b3 Compare February 22, 2017 18:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show slider range #454

Show slider range #454

cchan987 commented Jun 30, 2015

jgosmann Jun 30, 2015

jgosmann commented Jun 30, 2015

jgosmann Jun 30, 2015

cchan987 Jun 30, 2015

jgosmann commented Jun 30, 2015

cchan987 commented Jun 30, 2015

cchan987 commented Jun 30, 2015

jgosmann commented Jun 30, 2015

tcstewar commented Jun 30, 2015

cchan987 commented Jul 6, 2015

cchan987 commented Jul 6, 2015

tcstewar commented Jul 6, 2015

jgosmann commented Jul 6, 2015

tcstewar commented Jul 13, 2015

jgosmann commented Jul 13, 2015

cchan987 commented Jul 13, 2015

jgosmann commented Jul 13, 2015

tcstewar commented Jul 13, 2015

tbekolay commented Jul 13, 2015

tcstewar commented Jul 13, 2015

jgosmann commented Jul 13, 2015

tcstewar commented Jul 13, 2015

cchan987 commented Jul 29, 2015

cchan987 commented Jul 29, 2015

cchan987 commented Jul 29, 2015

tcstewar commented Jul 31, 2015

tcstewar commented Jul 31, 2015

tcstewar commented Jul 31, 2015

cchan987 commented Aug 10, 2015

Seanny123 commented Aug 15, 2015

tcstewar commented Aug 31, 2015

Show slider range #454

Are you sure you want to change the base?

Show slider range #454

Conversation

cchan987 commented Jun 30, 2015

jgosmann Jun 30, 2015

Choose a reason for hiding this comment

jgosmann commented Jun 30, 2015

jgosmann Jun 30, 2015

Choose a reason for hiding this comment

cchan987 Jun 30, 2015

Choose a reason for hiding this comment

jgosmann commented Jun 30, 2015

cchan987 commented Jun 30, 2015

cchan987 commented Jun 30, 2015

jgosmann commented Jun 30, 2015

tcstewar commented Jun 30, 2015

cchan987 commented Jul 6, 2015

cchan987 commented Jul 6, 2015

tcstewar commented Jul 6, 2015

jgosmann commented Jul 6, 2015

tcstewar commented Jul 13, 2015

jgosmann commented Jul 13, 2015

cchan987 commented Jul 13, 2015

jgosmann commented Jul 13, 2015

tcstewar commented Jul 13, 2015

tbekolay commented Jul 13, 2015

tcstewar commented Jul 13, 2015

jgosmann commented Jul 13, 2015

tcstewar commented Jul 13, 2015

cchan987 commented Jul 29, 2015

cchan987 commented Jul 29, 2015

cchan987 commented Jul 29, 2015

tcstewar commented Jul 31, 2015

tcstewar commented Jul 31, 2015

tcstewar commented Jul 31, 2015

cchan987 commented Aug 10, 2015

Seanny123 commented Aug 15, 2015

tcstewar commented Aug 31, 2015