Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

feat(sidenav): add gestures to sidenav to allow dragging the sidenav #6174

Closed
wants to merge 1 commit into from

Conversation

devversion
Copy link
Member

I'm currently working on the feature, which allows to drag the sidenav.

Fixes #1591 Fixes #35

@jelbourn jelbourn added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Dec 8, 2015
.on('$md.dragend', onDragEnd);

var style = getComputedStyle(element[0]);
var sidenavWidth = parseInt(style.width);
Copy link
Member

Choose a reason for hiding this comment

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

element[0].offsetWidth won't work better?

Copy link
Member Author

Choose a reason for hiding this comment

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

That won't work, because the sidenav is hidden, that's why offsetWidth doesn't work

@EladBezalel EladBezalel added the needs: manual testing This issue or PR needs to have some manual testing and verification done label Dec 21, 2015
@devversion
Copy link
Member Author

@EladBezalel Would be great if you test the provided demo 😄

@JumpLink
Copy link

I've create a module that replaces the original mdSidenav with the version of this pull request for testing:
https://gist.github.com/JumpLink/cd2b4188a54ec3f4cc7a

@devversion
Copy link
Member Author

Thanks for your work @JumpLink, but I think we should test my PR with fetching the current patch (https://patch-diff.githubusercontent.com/raw/angular/material/pull/6174.patch).

There is also a nice demo from me, so you only need to build the docs.

@EladBezalel
Copy link
Member

@devversion I tested it locally and it feels a bit weird, i assume you based on the bottom sheet code.
Bottom sheet feels more user friendly (take in account the swipe acceleration etc..)

@devversion
Copy link
Member Author

What do you mean?
I recorded it with the Chrome Dev Tools and there was a constant fps rate for the animation. It should drag similar to the bottom sheet. Please provide me more information

Here a little video

This even works on Android and IOS perfectly (smooth and user friendly)

@EladBezalel
Copy link
Member

I mean, bottom sheet have this extra space to be dragged to, also as i said, swipe the bottom sheet down real quick and it will be dismissed..

@devversion
Copy link
Member Author

But that makes no sense to add extra space to a sidenav (as we can see for example in the Android API 23, there was never a extra space for dragging. This will create more issues with the width of the sidenav. And the bottom sheet will close itself after dragging it a few pixels down. That shouldn't be implemented in a sidenav too.

@EladBezalel
Copy link
Member

OK, about the width i can agree with you, but as i mention earlier bottom sheet calculate the drag acceleration and dismiss if its was a fast swipe and it should be also on the sidenav (take a look at the google store sidenav)

@devversion
Copy link
Member Author

@EladBezalel can you test the swipe acceleration? :)

@EladBezalel
Copy link
Member

@devversion sure, later today I'll take a look!

@devversion
Copy link
Member Author

Great, thanks for your time!

@EladBezalel EladBezalel removed the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Dec 31, 2015

if (!isQuickDrag) {
if (isRightSidenav) isQuickDrag = lastDistance - ev.pointer.distanceX <= -accelerationBound;
else isQuickDrag = lastDistance - ev.pointer.distanceX >= accelerationBound;
Copy link
Member

Choose a reason for hiding this comment

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

wrap with brackets
also can be:

var distance = lastDistance - ev.pointer.distanceX;
isQuickDrag = isRightSidenav ?
              distance <= -accelerationBound :
              distance >= accelerationBound

Copy link
Member Author

Choose a reason for hiding this comment

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

I will use brackets ;)

@EladBezalel EladBezalel added needs: work and removed needs: manual testing This issue or PR needs to have some manual testing and verification done labels Dec 31, 2015
@EladBezalel
Copy link
Member

I would also change the right sidenav to be non-draggable cause the first button is left sidenav and it's in the dragging demo so it's the first button to press..

@devversion devversion force-pushed the wip/sidenav-dragable branch 2 times, most recently from 3a61ee8 to 550a200 Compare January 1, 2016 19:47
@ThomasBurleson
Copy link
Contributor

@devversion - what is the status of this PR? It appears outdated ?

@ThomasBurleson ThomasBurleson added in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: more info The issue does not contain enough information for the team to determine if it is a real bug and removed needs: review This PR is waiting on review from the team labels Apr 19, 2016
@ThomasBurleson ThomasBurleson added this to the 1.1.1 milestone Apr 19, 2016
@devversion
Copy link
Member Author

@ThomasBurleson It is not outdated - I will rebase the next days.

We can better say, it's currently blocked - because once this feature is landed, the siidenav will not allow scrolling for iOS anymore.

This is caused by #7311 and a fix is already pending #7857

@devversion devversion added the needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved label Apr 19, 2016
@ThomasBurleson ThomasBurleson modified the milestones: 1.1.1, Backlog Apr 21, 2016
@devversion devversion added the Blocked Progress on this issue is blocked. Primarily used for PRs that are blocked by presubmit feedback. label Apr 21, 2016
@ThomasBurleson ThomasBurleson modified the milestones: - Backlog, Deprecated May 26, 2016
@devversion
Copy link
Member Author

FYI: This PR will be deprecated and re-submitted, once #7857 has landend.

@devversion devversion removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: more info The issue does not contain enough information for the team to determine if it is a real bug labels May 27, 2016
@ThomasBurleson
Copy link
Contributor

This issue is closed as part of our ‘Surge Focus on Material 2' efforts.
For details, see our forum posting.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
- Lots of Comments Blocked Progress on this issue is blocked. Primarily used for PRs that are blocked by presubmit feedback. needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants