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

Slider #572

Merged
Merged

Conversation

mansona
Copy link
Collaborator

@mansona mansona commented Nov 24, 2016

So this is the start of the Slider implementation for #249. I'm submitting this as a PR way early just so I can signal to people that I am working on it and we can start any discussions that we need to.

Last time I took a look at this I noticed that the JS backing the slider needed quite a bit of work. There are some serious performance issues because (if I remember correctly) it doesn't have an "internal concept of the slider value".

My approach will be to refactor the javascript and hopefully a solution will fall out of that refactor.

@miguelcobain the Angular material implementation has a demo for a verticle slider https://material.angularjs.org/latest/demo/slider. Do you want to block this PR (and 1.0) on a verticle slider implementation or do you just want parity with the pre 1.0 version of Ember-Paper?

@miguelcobain
Copy link
Collaborator

@mansona v1 won't wait for new features. Of course, if we happen to have PRs for new features, they will be accepted (like it happened with Chips).

I remind you that AM 1.0.6 doesn't have vertical sliders, which is the version we're using.
To use vertical sliders you'd have to backport styles from more recent AM versions.

I'd say that just horizontal sliders would be good and simpler for now.
Thanks for working on this!

@miguelcobain miguelcobain added this to the 1.0 milestone Nov 24, 2016
@mansona
Copy link
Collaborator Author

mansona commented Dec 9, 2016

So after a lot of pain 😠 I have finally figured out what was causing the Jank while sliding the sliders!! I thought it was something to do with the JavaScript and related to how we were applying the left style value jumping through a few functions but it was simply a CSS transition thing 😫

I have only backported the particular section of the new CSS required so as not to complicate things, for now 👍

We do have one last strange behaviour that is impacting the end-user-experience, the "drag" event seems to be locked to only the vertical space taken up by the slider. You can see this demonstrated in the following video: https://cl.ly/2U220q062A2y keep an eye on where my mouse is in the vertical axis in both cases.

The only trick I know of to get this component to "take over" the mouseMove is to use HTML5 Drag and Drop. Just investigating what Angular Material do, they actually make use of drag and drop 🎉 ... but this is where things get pretty strange...

Implementing the same thing with drag and drop looks like this: https://cl.ly/130x0q1a1T0I

I have found a hack that can get around the above strange experience by adding the following code:

    let dragIcon = document.createElement('img');
    dragIcon.setAttribute('style', 'width:1px;height:1px;border:none;display:block');
    dragIcon.src = 'data:image/gif;base64,R0lGODlhAQABAIAAAP///wAAACH5BAEAAAAALAAAAAABAAEAAAICRAEAOw==';
    event.dataTransfer.setDragImage(dragIcon, 0, 0);

which results in a successful Jank-free great user experience... but feels a bit "messy" from a code perspective 😖 https://cl.ly/3I3r170F3u0Y

I wasn't going to commit these drag and drop experiments but they essentially work and it's just down to a (code) style decision now. If you are happy with these minor hacks then merge away and this could be considered done... I guess... I have a few improvements I would like to make so that discrete sliders work much better but that can really wait until after 1.0 release.

Any thoughts @miguelcobain?

@@ -77,6 +77,9 @@ export default Component.extend(FocusableMixin, ColorMixin, {

active: false,
dragging: false,
enabled: computed('disabled', function() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use computed.not('disabled')

@mansona
Copy link
Collaborator Author

mansona commented Dec 10, 2016

@miguelcobain thanks for that 👍 I've been using ember-truth-helpers for too long that I've fallen out of practice of Ember.computed 😂

@mansona
Copy link
Collaborator Author

mansona commented Dec 22, 2016

So as far as I can see this is done and can be merged 🎉 Like I said due to the amount of stuff that needs to be done pre 1.0 it's probably best to leave any of the "nice to have" features until later.

Unless you want me to bring it more in line with the Angular Sliders, I can implement the pips and the better user experience for the discrete slider, just let me know how you want to proceed 👍

@mansona
Copy link
Collaborator Author

mansona commented Jan 9, 2017

Hey @miguelcobain I've implemented the slider using hammer.js and it's working beautifully. I am pretty happy that the hack is out of there now too 👍

I "borrowed" my hammer implementation from the only other implementation I could find in the codebase, paper-switch, so let me know if any of the code style needs adjusting.

Oh and I rebased on master because it needed one 👍

@miguelcobain miguelcobain merged commit 7b9cd7c into adopted-ember-addons:master Jan 9, 2017
@miguelcobain
Copy link
Collaborator

Thank you @mansona. Nice work!

@mansona mansona deleted the feature/slider branch January 9, 2017 16:04
@mansona
Copy link
Collaborator Author

mansona commented Jan 9, 2017

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants