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

Widget values now use value formatter #562

Merged
merged 12 commits into from
Mar 23, 2016
Merged

Widget values now use value formatter #562

merged 12 commits into from
Mar 23, 2016

Conversation

philippjfr
Copy link
Member

Currently the values displayed in the slider widget text box and dropdown menu are derived from the raw value (with some rounding applied). However as suggested in #548 the widgets should also use the dimensions value formatter if specified. This allows timestamps to be formatted correctly, for example, ensuring that you don't get mismatching labels between the title and the widget. The only issue with this approach is that the formatted version can often be longer and therefore become unreadable. Before merging I'd like to brainstorm a little bit about what we can do for very long labels. Here is an example:

screen shot 2016-03-22 at 11 45 52 am

Note I also reindented the jinja template because it was a mess.

@jbednar
Copy link
Member

jbednar commented Mar 22, 2016

I think that even if it's longer, it's better to have it show a meaningful value. Right justifying it would help a bit, since the fastest-changing bit is typically on the right. I guess we could auto-size (at least, auto-increase) the widget based on the first value's width plus a tiny extra margin? And/or allow the user to specify the width as a style option?

@philippjfr
Copy link
Member Author

I'd prefer not to adjust the widget width as screen real-estate is important and allocating more than 25% of the screen width to the widget seems excessive. In future when designing more customizable widgets we should definitely keep this in mind though.

For now I'm playing with solutions that automatically adjust the size of the font. Something like this works reasonably well:

if (txt.val().length > 10) {
    text.css('font-size', 100-Math.min(text.val().length*2.4, 50)+'%');
}

screen shot 2016-03-22 at 2 03 41 pm

@jlstevens
Copy link
Contributor

Adjusting the fonts seems fine for now.

The text boxes on the right display the values make it conceivable that values could be edited and then set directly. For other values such as the dates, textual editing wouldn't be particularly easy so I can imagine a different type of slider that takes up the full width that only shows the value when you hover or drag the slider bar. I suppose the problem with that idea is you wouldn't be able to see the selected value immediately (e.g after exporting the notebook).

Anyway, this is a good usability improvement and I'm happy to merge if we agree changing the font size is the best thing for now.

@jbednar
Copy link
Member

jbednar commented Mar 22, 2016

I like having the actual values shown on the page, not just for hovering. But it seems like there is always going to be a tension between readability and width on the page, with the layout above. Is it not possible to use a more compact layout, like in this mockup:

image

@philippjfr
Copy link
Member Author

That was what the widgets looked like originally. We moved away from that deliberately due to alignment issues and because it only works well for short dimension names.

@jbednar
Copy link
Member

jbednar commented Mar 22, 2016

Previously, did the space for the value automatically spread to cover the space remaining after the dimension name? The dimension names are up to the user to choose, and so it seems like they can decide to favor having long names or more room to edit values, and as long as they don't demand both at once for the same dimension, everything would fit into less space.

@philippjfr
Copy link
Member Author

Previously, did the space for the value automatically spread to cover the space remaining after the dimension name?

No you're right that wasn't the case before, the question remains what happens if the name is so long it squashes the text area entirely. I guess you could limit it to a 80/20 or similar and truncate the dimension name if it doesn't fit in that space.

I think we should revisit this once we have decided on what we want to do about widgets in general. Getting the UI right is fiddly and will take time I don't want to commit at the moment. Since we already depend on jquery-ui we could however allow some basic resizing. By adding a few lines I was able to get something working, you can see the behavior here: https://sendvid.com/pngy9ip0

@jbednar
Copy link
Member

jbednar commented Mar 22, 2016

Resizing would be very useful...

@jlstevens
Copy link
Contributor

@philippjfr That looks pretty good!

That seems like a nice improvement for now. The reason I suggested showing values on hover because then long dimension names and long value names wouldn't be competing for space.

@jbednar
Copy link
Member

jbednar commented Mar 22, 2016

Sure, those limits sound reasonable. Yes, hovering does help avoid competition between long names and long values, but both of them are important to have as a record and a manifest of what's being shown in the plot, so one would really like both of them to be onscreen at all times. I would hope that with the above re-organization plus the ability to drag the area to make it bigger, we wouldn't get many complaints about unusable widgets anymore...

@philippjfr
Copy link
Member Author

I've restricted the functionality a little bit since that demo because there was lots of potential for strange behavior. Here's what it looks like now: https://sendvid.com/o251s606

I think that gives sufficient control to actually see the values if they are really long and we can work out better solutions for sizing, moving and otherwise improving the widgets once we've decided what libraries we should build it on.

@jbednar
Copy link
Member

jbednar commented Mar 23, 2016

Sounds good for now.

@philippjfr
Copy link
Member Author

@jlstevens if you agree this is ready to merge now.

@jlstevens
Copy link
Contributor

Ok great! Merging...

@jlstevens jlstevens merged commit 1c7e285 into master Mar 23, 2016
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.

None yet

3 participants