JuliaGizmos / InteractBase.jl

Build interactive HTML5 widgets in Julia
Other
27 stars 23 forks source link

Make rangeslider the default #103

Closed piever closed 6 years ago

piever commented 6 years ago

@rafaqz as we were discussing this in another issue: here I'm making rangeslider the default. It seems overall to be a better implementation as it allows several handles (for example to select a range) and having the value on top of the handle (which I think is better than on the side). The main downside is that the new one doesn't accept keyboard input (meaning it can't be moved with arrows, see https://github.com/leongersen/noUiSlider/issues/724) even though that seems to be a planned functionality.

If anybody is not in favor of this change, this is a good place to discuss.

I'm also curious to know whether some users would still prefer to use nativeslider or if it can be deprecated.

codecov-io commented 6 years ago

Codecov Report

Merging #103 into master will increase coverage by 0.05%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #103      +/-   ##
=========================================
+ Coverage   96.44%   96.5%   +0.05%     
=========================================
  Files           9       9              
  Lines         394     400       +6     
=========================================
+ Hits          380     386       +6     
  Misses         14      14
Impacted Files Coverage Δ
src/defaults.jl 100% <ø> (ø) :arrow_up:
src/slider.jl 98.21% <100%> (+0.21%) :arrow_up:
src/input.jl 97.14% <100%> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 29ca32a...f17b688. Read the comment docs.

rafaqz commented 6 years ago

Apologies I havent quite got around to dealing with the styling with all the 1.0 updates I've been doing!

I have a few minor styling concerns about the new rangeslider. The sliding square is big! when the sliders are small it takes up a large proportion of the slider. But that's easily fixed. The tooltip also takes up a lot of vertical space. When I was putting together an interface with up to 80 sliders I went for the standard slider to keep the interface compact, so it might be good to keep that format as an option?

piever commented 6 years ago

You're probably right that I should keep both, esp. as the native HTML one is actually more in line with the philosophy of the package (pure HTML widget with extra CSS for style). What do you suggest in terms of names? I'm not thrilled by nativeslider and slider. Maybe slider (HTML) and rangeslider (noUIslider) is not so bad after all, as long as the rangeslider is well documented.

piever commented 6 years ago

Apologies I havent quite got around to dealing with the styling with all the 1.0 updates I've been doing!

No problem! Check out #89 for the layout, maybe we can coordinate efforts there.

rafaqz commented 6 years ago

Hah those are the exact lines I was changing.

My branch has all of that styling replaced with classes so the details are in the CSS file. Basically I'm imposing a "no styling in code" rule but I wasn't sure if everyone is on board with that.

It can easily incorporate those changes.

piever commented 6 years ago

Basically I'm imposing a "no styling in code" rule but I wasn't sure if everyone is on board with that.

That's definitely a healthier approach, when you have time do make a PR potentially incorporating the fixes from @djsegal / coordinating with him!

rafaqz commented 6 years ago

Hopefully soon, whenever this 1.0 CI nightmare is over

Is rangeslider specifying that the slider can have two values ie a range? then rangeslider is a good name! :)

piever commented 6 years ago

Yes, it can have two or more values. Related question then: should rangeslider default to selecting a range then? Say value = [extrema(vals)...] by default?

rafaqz commented 6 years ago

Cool. I had to look up what extrema() did.

I'm not sure what the default should be... In some ways it is also a nicer general slider than slider() is. I would probably use it for single sliders in simple layouts, just avoid it when I need to cram 100 sliders onto a page. I don't have any personal uses for sliders that specify ranges, but maybe I will find some now that it exists...

piever commented 6 years ago

I've pushed a bunch of fixes to the normal slider so that now it has orientation=:vertical as well as support for unitful arrays and I'm getting more and more convinced to keep both (and export both).

The two questions are:

Materialize.css seems to have come to a similar compromise, see their docs