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

paper-checkbox-light #145

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

paper-checkbox-light #145

wants to merge 7 commits into from

Conversation

bicknellr
Copy link
Contributor

@bicknellr bicknellr commented Aug 8, 2016

Diff with master:
d38cacc...paper-checkbox-light

Key differences with paper-checkbox:

  • No label.
  • Does not support use in an iron-form (i.e. doesn't include IronFormElementBehavior).
  • Does not support validation (i.e. doesn't include IronValidatableBehavior).

@bicknellr
Copy link
Contributor Author

bicknellr commented Aug 8, 2016

I'll remove the validation tests soon since they no longer apply; the line-height test failing was unexpected though.

<h3>Checkboxes can be checked or unchecked, or disabled entirely</h3>
<demo-snippet class="centered-demo">
<template>
<paper-checkbox-light></paper-checkbox-light>
Copy link
Contributor

@notwaldorf notwaldorf Aug 8, 2016

Choose a reason for hiding this comment

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

I would also add a demo for "what should you do if you want a label"

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I would also bring back the subtitle examples, but with flexbox or whatever you need. Maybe just have a section "Checkboxes with labels", and do a bunch of examples? Like single line, multi-line-align-top, multi-line-align-center?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I added them back.

@bicknellr
Copy link
Contributor Author

paper-checkbox-light needs to include the ink sizing stuff from #135; hopefully that won't slow it down much.

@bicknellr
Copy link
Contributor Author

bicknellr commented Aug 9, 2016

Also, noink doesn't work.

@bicknellr bicknellr force-pushed the paper-checkbox-light branch from c30387f to b9400af Compare August 9, 2016 18:06
@bicknellr
Copy link
Contributor Author

bicknellr commented Aug 9, 2016

Added noink support and fixed some associated keyboard vs. pointer ripple stuff. Also, I copied over the ink sizing code from #135 and delayed it until the ripple is first created. That would probably be ok to put in regular paper-checkbox also, but I'll save that for a later PR. The diff's a bit gnarlier now, sorry. :/

@bicknellr bicknellr force-pushed the paper-checkbox-light branch from 8f29b9f to 14b7f6a Compare August 9, 2016 23:07
@bicknellr
Copy link
Contributor Author

As usual, Firefox and Selenium have reacted violently; everything seems ok locally though.

* Focus events occuring less than this value (in milliseconds) after a
* 'down' event are considered to be the result of a pointer.
*/
__POINTER_FOCUS_WINDOW: 30,
Copy link
Contributor

Choose a reason for hiding this comment

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

Uhmmmm is this the alternative to receivedFocusFromKeyboard? Does it fix the problem where switching tabs focuses the element? Should we port this to IronControlState as well?

/cc @cdata

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uhmmmm is this the alternative to receivedFocusFromKeyboard?

Yeah, the behavior seems pretty much the same (as far as I can tell) except paper-checkbox-light also ripples when toggling with the keyboard after you initially focused it with a pointer. At some point I'd also like to make the persistent ripple come back when you toggle with the keyboard but I don't think it's strictly necessary for this PR, given regular paper-checkbox doesn't do this either.

Does it fix the problem where switching tabs focuses the element?

Shouldn't it receive focus when you switch back to the page?

@bicknellr
Copy link
Contributor Author

The builds are now lying to us... They failed, even though it's just the usual Firefox + Selenium and Sauce Connect timeout problems, but they're reporting as having passed.

@notwaldorf
Copy link
Contributor

@bicknellr Yeah, i merged a PR yesterday that was all green, but the tests were legit broken 😭

@bicknellr
Copy link
Contributor Author

Note to self: #149
Normal paper-checkbox also needs to have ink size calculation delayed until focus or down.

@bicknellr
Copy link
Contributor Author

ping @notwaldorf

var isSpace = event.key === ' ' || event.code === 'Space' || event.keyCode === 32;
var isEnter = event.key === 'Enter' || event.code === 'Enter' || event.keyCode === 13;
if (isSpace || isEnter) {
event.preventDefault();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need stopImmediatePropagation too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so; preventDefault seems to be enough to stop scrolling.

@notwaldorf
Copy link
Contributor

minor comments left. If the tests are green and the demos look ok, then LGTM

@bicknellr bicknellr force-pushed the paper-checkbox-light branch from 1e020b8 to 3a14255 Compare September 8, 2016 01:33
@bicknellr
Copy link
Contributor Author

I added 3a14255...paper-checkbox-light.

@bicknellr
Copy link
Contributor Author

Ok, figured out why the tests were failing in some older browsers: I had used method syntax in an object literal.

@bicknellr
Copy link
Contributor Author

@notwaldorf mind taking another look?

@LarsDenBakker
Copy link

Running into some performance issues with paper-checkbox, what's the status of this?

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

Successfully merging this pull request may close these issues.

4 participants