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

Fixes touch interactions for slider #8449

Merged
merged 3 commits into from
Apr 27, 2016
Merged

Conversation

Owlbertz
Copy link
Contributor

Fixes the general touch support for slider plugin and I guess also for other plugins since the error seemed to affect all plugins there.

However, sliding still kinda bugs when vertical slider is used on touch devices (doesn't happen in Chrome emulation...) - I will fix that within the next days.
Edit: Clicking the slider's track actually works well, just the dragging part needs some work.

Did this work without `new` actually?
The touch util seems to emit wrong values for pageX and pageY. These values seem to miss the window.scroll amount.
I added an switch to check for this case so the scrolled amount will be considered as well.
@ghost
Copy link

ghost commented Mar 23, 2016

Damn this is a nice one! Thanks for commiting!

Just curious, are you contracted by Zurb Foundation? If not they should :)

@Owlbertz
Copy link
Contributor Author

Should be touch-usable now. Although the slider handle is quite hard to hit on touch. Maybe there should be some CSS making it bigger on small screens by default?

@Owlbertz
Copy link
Contributor Author

@Silvester20 would you be able to test this one?

@rafibomb
Copy link
Member

Thanks @Owlbertz We'll be testing this out! Would like to to get this into the next patch

@ghost
Copy link

ghost commented Mar 25, 2016

@Owlbertz Yeah sure, but don't know how to test this. My site is using the foundation.min.js & foundation.min.css from the complete download package.

And I assume when I redownload the complete package the files doesn't have changed, still the same foundation.min.js as the release of 6.2.0, without your commits right?. So how can I test this?

@rafibomb
Copy link
Member

@Silvester20 You can checkout the branch here: git checkout -b Owlbertz-touch-slider develop and pull those changes to test it. We'd love to see more people confirming the fix so we can merge it in faster.

@rafibomb rafibomb mentioned this pull request Apr 10, 2016
6 tasks
@andycochran
Copy link
Contributor

@Owlbertz—regarding "hard to hit on touch"—since small screen ≠ touch screen, we could use what-input.js to know when the user is trying to touch the slider and adjust the style with something like this:
http://codepen.io/andycochran/pen/VaQzzz

@Owlbertz
Copy link
Contributor Author

@andycochran Yeah, I had something like this on my mind as well. Looks promising.

@kball
Copy link
Contributor

kball commented Apr 26, 2016

@Silvester20 were you able to test this out? Alternatively, @andycochran can you try it out and see if it works as expected?

@andycochran
Copy link
Contributor

@kball This is only a JS change, no CSS. So you'd be a better judge. Did we want this to address the "hard to hit on touch" problem, or should we document that in a separate issue?

@Owlbertz
Copy link
Contributor Author

Since this fixes an actual issue I think the "convenience stuff" could be done later on, in a separate issue.

@kball
Copy link
Contributor

kball commented Apr 27, 2016

k tested it out, seems good. Merging.

@kball kball merged commit 11eb851 into foundation:develop Apr 27, 2016
@kball
Copy link
Contributor

kball commented Apr 27, 2016

@Owlbertz does this fix any outstanding filed issues that you are aware of?

@Owlbertz
Copy link
Contributor Author

Owlbertz commented Apr 27, 2016

At least #8666 which I recently referenced. I do believe there was something else reported by @Silvester20 Edit: This one #8413 (comment).

@ghost
Copy link

ghost commented Apr 27, 2016

I can only test this with a compiled javascript (*.js) file. But I trust you guys :)

@sjransom
Copy link

This seems to fix some touch issues I've been having. Have tested on latest iOS and Android, many thanks @Owlbertz

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

Successfully merging this pull request may close these issues.

5 participants