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

[LeftNav] Add iOS momentum scroll #2946

Merged
merged 1 commit into from
Feb 19, 2016
Merged

[LeftNav] Add iOS momentum scroll #2946

merged 1 commit into from
Feb 19, 2016

Conversation

ffxsam
Copy link
Contributor

@ffxsam ffxsam commented Jan 15, 2016

It really bugged me that there was no touch scroll (momentum) on a few Material UI elements. Makes the interface feel a bit dated.

@ffxsam
Copy link
Contributor Author

ffxsam commented Jan 15, 2016

I was able to add it to DropDownMenu, no problem. But I applied the SAME exact style to LeftNav, and well.. look at this weirdness:

different_styles

The first one circled is LeftNav. The second one is an activated DropDownMenu. Notice how the LeftNav style is webkit-overflow-scrolling and the other is -webkit-overflow-scrolling, yet the code for both is _exactly the same_. What the heck?

@alitaheri
Copy link
Member

I'm guessing this must be related to auto-prefixer. @oliviertassinari Thoughts?

@oliviertassinari
Copy link
Member

What the heck?

I agree, I'm wondering if this has nothing to do with https://github.com/rofrischmann/inline-style-prefixer.

@ffxsam
Copy link
Contributor Author

ffxsam commented Jan 21, 2016

That's exactly it. I didn't know about this at before now. It should be WebkitOverflowScrolling, not webkitOverflowScrolling. Fixed, and tested on iPhone 5s, iPad Air, and iOS Simulator (iPad). Anyone else can feel free to double check before merging.

@@ -264,6 +264,7 @@ const LeftNav = React.createClass({
transition: !this.state.swiping && Transitions.easeOut(null, 'transform', null),
backgroundColor: theme.color,
overflow: 'auto',
WebkitOverflowScrolling: 'touch',
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment like Add momentum scrolling for iOS?

@oliviertassinari
Copy link
Member

@ffxsam That feel much better on iOS 👍!
Can you have a look at my comment and squash down the commit then so we can merge?

@oliviertassinari oliviertassinari added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Jan 23, 2016
@ffxsam
Copy link
Contributor Author

ffxsam commented Jan 23, 2016

What about moving this style to the DropDownMenu component?

Where exactly? I tried putting in the root style but it had no effect. I'm still not familiar enough with the code structure to know the proper place to put things. I think I tried putting it in that component and it didn't work..

Also, you'll have to tell me how I should squash my commits, it's not something I do very often and I don't want to mess up the branch. :)

@oliviertassinari
Copy link
Member

@ffxsam
Copy link
Contributor Author

ffxsam commented Jan 23, 2016

Nope, that didn't work. I'll put it back. The overflow scrolling property has to be in a very specific spot.. I put it back to where it works.

@ffxsam
Copy link
Contributor Author

ffxsam commented Jan 23, 2016

I tried to squash/rebase.. I think I messed up. I might delete this PR and start over again.

@oliviertassinari
Copy link
Member

Nope, that didn't work. I'll put it back.

My bad. Applying this style to the animation component sounds wrong to me.
What's your use case for it?

@ffxsam
Copy link
Contributor Author

ffxsam commented Jan 28, 2016

What do you mean by use case? It's the only place that WebkitOverflowScrolling seems to have worked. Is there another place you'd recommend putting it?

@oliviertassinari
Copy link
Member

Is there another place you'd recommend putting it?

I have to investigate. But adding this style inside the animation component doesn't feel right. The /popover-animation-from-top.jsx component should only have on responsibility: take care of the animation.

@mbrookes mbrookes changed the title Added iOS momentum scroll to DropDown Menu, and.. {DropDownMenu] Add iOS momentum scroll Feb 4, 2016
@oliviertassinari
Copy link
Member

@ffxsam Any update on this?
What do you think about submitting a PR for the LeftNav component? This part is good and can be merged IMHO 👍.
For the Popover we need to find a better approach.

@ffxsam
Copy link
Contributor Author

ffxsam commented Feb 19, 2016

Sorry, been totally swamped lately! Please keep this open.. I will return to it as soon as I can and merge the LeftNav. And then I'll take a second look at Popover.

@ffxsam
Copy link
Contributor Author

ffxsam commented Feb 19, 2016

Ok, I removed it from the Popover. I would normally squash the commits, but I know once you do that, you can't re-push.

@mbrookes
Copy link
Member

I would normally squash the commits, but I know once you do that, you can't re-push.

git push -f

@ffxsam
Copy link
Contributor Author

ffxsam commented Feb 19, 2016

Ok. I thought that was bad practice. Should I squash and force push then?

@mbrookes
Copy link
Member

Yes please.

@mbrookes mbrookes changed the title {DropDownMenu] Add iOS momentum scroll [DropDownMenu] Add iOS momentum scroll Feb 19, 2016
@mbrookes mbrookes added PR: Review Accepted and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Feb 19, 2016
@oliviertassinari oliviertassinari changed the title [DropDownMenu] Add iOS momentum scroll [LeftNav] Add iOS momentum scroll Feb 19, 2016
@oliviertassinari
Copy link
Member

@ffxsam Thanks! That's much better 👍:
fevr 19 2016 12 10

oliviertassinari added a commit that referenced this pull request Feb 19, 2016
@oliviertassinari oliviertassinari merged commit 6276d67 into mui:master Feb 19, 2016
@ffxsam ffxsam deleted the ios-momentum-scrolling branch February 22, 2016 22:36
@zannager zannager added the docs Improvements or additions to the documentation label Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants