Skip to content
This repository has been archived by the owner on Aug 29, 2023. It is now read-only.

Material-switch needs refactoring #321

Closed
marcysutton opened this issue Sep 24, 2014 · 15 comments
Closed

Material-switch needs refactoring #321

marcysutton opened this issue Sep 24, 2014 · 15 comments
Labels
a11y This issue is related to accessibility
Milestone

Comments

@marcysutton
Copy link
Contributor

material-switch currently extends material-checkbox and material-radio, automatically injecting markup and attributes that are unpredictable and hard to test. It is also incompatible with ngAria. See this screenshot of the rendered markup:

Material switch markup in Chrome Developer Tools

Unlike checkbox and radio, attributes are added to an interior element. There are actually duplicate attributes for tabIndex and aria-label, and ngModel attributes like 'yup' don't trigger an update to aria-checked when ngAria is in use. Making this component more consistent with the others would make our lives much easier.

@marcysutton marcysutton added a11y This issue is related to accessibility type: bug labels Sep 24, 2014
@miguelcobain
Copy link

Also, it looks like the appearance doesn't conform the specs, right?

Current:
material angular switches

Spec:
material switches from spec

@plato-cambrian
Copy link

You can also observe issue #548 at the slider/switch demo page if you scroll (at least in firefox 33)

@ThomasBurleson ThomasBurleson added this to the 0.6.0-rc1 milestone Nov 14, 2014
@ThomasBurleson ThomasBurleson modified the milestones: 0.6.0, 0.7.0-rc1 Dec 8, 2014
@ajoslin
Copy link
Contributor

ajoslin commented Dec 10, 2014

Fixed via 8bc9461

@ajoslin ajoslin closed this as completed Dec 10, 2014
@miguelcobain
Copy link

Impressive work @ajoslin! Thanks!

@miguelcobain
Copy link

I've noticed some problems related with dragging on mobile. You can test this using Chrome touch emulation.

We should probably use Hammer for these drag events?

ajoslin added a commit that referenced this issue Dec 10, 2014
@ajoslin
Copy link
Contributor

ajoslin commented Dec 10, 2014

I didn't test heavily enough on touch devices. Pushed a fix.

@ajoslin
Copy link
Contributor

ajoslin commented Dec 10, 2014

Also, to answer your question .. we are in the process of moving away from HammerJS because it has old android bugs, and we only use its drag & swipe functionality.

@miguelcobain
Copy link

It's working!

I'm not trying to be picky here, but ideally we should look at the position of the final drag and decide which is closer: on or off.

This is what Polymer does, I think. But I don't know how the spec handles this detail.
It is really usable and good looking right now!

@ajoslin
Copy link
Contributor

ajoslin commented Dec 10, 2014

We do do this!

If you move the switch less than 5px, we count it as a click/tap and toggle.

if you move the switch over 5px but less than 50% of the way to the next state, we do nothing.

If you move the switch over 50% of the way to the other state, it toggles.

@ajoslin
Copy link
Contributor

ajoslin commented Dec 10, 2014

And please be picky! Picky people make progress.

@miguelcobain
Copy link

Definitely agree. :)

Does the demo page reflect this fix?
I can't reproduce the "If you move the switch less than 5px" scenario on the demo page.
Basically I move the less I can and it counts as a switch change.
Doesn't matter what I do, every click I make gets the state changed.

Chrome 39 on Linux.

Edit: forget it. I wasn't actually moving 5px.
Polymer doesn't do this. Example.

I really like this, but maybe a little less threshold would be better.
The thing is that 5px and 50% are too close when the switch is this small.
Maybe 3px?
It feels counter intuitive to move the switch just a bit and get it toggled. Maybe I changed my mind and stopped dragging before 5px? Or maybe I changed my mind and moved it back to the original place and hit the 1 to 5px threshold? In both these scenarios the outcome doesn't reflect my intention.
Maybe this is why polymer didn't do this, I don't know.

This is an edge case, anyway. Great job.

@ajoslin
Copy link
Contributor

ajoslin commented Dec 10, 2014

This is good feedback.

Or maybe I changed my mind and moved it back to the original place and hit the 1 to 5px threshold?

Maybe 3px?

I think I'll just make it so "only count it as a click if a move event didn't happen."

This will fix all the problems.

@ajoslin
Copy link
Contributor

ajoslin commented Dec 10, 2014

I ended up allowing the user to move 1px and still count as a click.

@miguelcobain
Copy link

Perfect!

@slaat
Copy link

slaat commented Jun 16, 2015

I still have a couple of issues with the switch:

If moved more than 50%, but not 100%, it does not toggle
If moved to the other side (100%), but the cursor does not hover on another div, it does not toggle (drag it from left to right and hover the text on release, it does not toggle)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a11y This issue is related to accessibility
Projects
None yet
Development

No branches or pull requests

6 participants