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

feat(slide-toggle): add drag functionality to thumb #750

Merged
merged 3 commits into from
Jul 14, 2016

Conversation

devversion
Copy link
Member

This adds the dragging functionality to the slide-toggle.

It works pretty neat. Also tested on older touch devices. Seems like HammerJS is doing a pretty good job.

FYI: I intentionally didn't use the class / style bindings, because we don't want to trigger a change detection for each Drag Event.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jun 23, 2016
@devversion
Copy link
Member Author

@jelbourn Not sure how HammerJS will run on the users Environment later. What were your thoughts?


hammerManager.on('panstart', this._onDragStart.bind(this));
hammerManager.on('panmove', this._onDrag.bind(this));
hammerManager.on('panend', this._onDragEnd.bind(this));
Copy link
Member

Choose a reason for hiding this comment

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

Why are you adding the events directly instead of using handlers defined via the template, e.g.,

<div class="md-slidetoggle-thumb" (dragstart)="..." ...>

?

Check out @iveysaur's PR #779 for the slider; she's using the baked-in gesture stuff

Copy link
Member

Choose a reason for hiding this comment

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

Also, I tried this on my phone (Galaxy S6) and it was a bit sluggish. I wonder if we should throttle these drag events (would we need a throttle function in the library, though...)

Copy link
Member

Choose a reason for hiding this comment

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

Actually, it's fine for now- can you just add a TODO to throttle the events later?

Copy link
Member Author

@devversion devversion Jul 8, 2016

Choose a reason for hiding this comment

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

Why are you adding the events directly instead of using handlers defined via the template, e.g.,

Yeah great idea - as we discussed on Slack

Also, I tried this on my phone (Galaxy S6) and it was a bit sluggish. I wonder if we should throttle these drag events (would we need a throttle function in the library, though...)

Yeah I fixed that by calculating the absolute difference (as done in the slider)

Actually, it's fine for now- can you just add a TODO to throttle the events later?

Not sure, why throttling is necessary here? It seems like throttling would probably make it less smooth.

@devversion
Copy link
Member Author

@jelbourn Addressed your comments
@kara I've implemented the slide event as discussed (also the cleanup)

One thing I've noticed:

  • When calculating the percentage for the drag absolutely, there will be a gap, when starting to drag
    It will look like the drag starts in a higher x position.
  • Dragging / Sliding should be calculated relatively, by using the distance of the Hammer event.
    The difference is, that the distance will take care of the cursor position (whether you click at the start / end of the thumb)

    This means, that we miss those "pixels" for the calculation and the drag would not feel smooth / perfectly.
  • After some investigation I finally found the perfect way for the thumb drag functionality and it seems like works like a charm.

cc. @iveysaur (Also noticed this in the Slider component)

// Once the thumb container is being dragged around, we remove the transition duration to
// make the drag feeling fast and not delayed.
&.md-dragging {
transition-duration: 0ms;
Copy link
Member

Choose a reason for hiding this comment

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

Could just be 0 without the ms (hopefully the stylelint doesn't want this?)

Copy link
Member Author

@devversion devversion Jul 9, 2016

Choose a reason for hiding this comment

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

Seems like it's not allowed to have just zero here, because it needs a time unit.

@@ -58,6 +59,13 @@ export class MdSlideToggle implements AfterContentInit, ControlValueAccessor {
private _hasFocus: boolean = false;
private _isMousedown: boolean = false;
private _isInitialized: boolean = false;
private _slideRenderer: MdSlideToggleRenderer = null;

// State of the current drag, which holds required variables for the drag.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: use /** */ style comments for member / method descriptions

@jelbourn
Copy link
Member

LGTM!

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Jul 14, 2016
@kara kara merged commit 25b4f21 into angular:master Jul 14, 2016
@devversion devversion deleted the feat/drag-functionality branch July 14, 2016 22:12
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants