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

Dan chadwick paper input #326

Merged
merged 42 commits into from
Apr 12, 2016
Merged

Dan chadwick paper input #326

merged 42 commits into from
Apr 12, 2016

Conversation

miguelcobain
Copy link
Collaborator

I've fixed the focusing logic.
Also removed noFloat. It doesn't do what we expected it to from what I've read from the angular material source. So I opted out that feature.

The only thing missing is to make this a one-way input. Let's discuss this.

DanChadwick and others added 30 commits March 22, 2016 21:23
…1.1 to 1.0.6.

The app/styles/backports/paper-input.scss file can be deleted when updated to angular material 1.0.6.
… either an array of messages or hashes containing messages.
@miguelcobain miguelcobain force-pushed the DanChadwick-paper-input branch from 6fb36e3 to 5b5bffa Compare April 5, 2016 23:34
@miguelcobain
Copy link
Collaborator Author

Validation tests are missing.

@DanChadwick
Copy link
Contributor

Also, I am now seeing the placeholder on top of the number input elements. I think this is a regression, unless we weren't rendering number html5 elements before. Either way, its broken. See the demo app Minimum Value and Maximum Value examples

@DanChadwick
Copy link
Contributor

Regarding html5 attributes, I think we should still include them because a) they are in {{input}} element and b) they might well be useful.

@DanChadwick
Copy link
Contributor

Dummy app is out-of-date WRT focusin->focus and focusout->blur. Also out-of-date wrt attr.xxx, if indeed these are removed.

@DanChadwick
Copy link
Contributor

"Value should be even and equal to 4" does not work in dummy app.

@miguelcobain
Copy link
Collaborator Author

Regarding html5 attributes, I think we should still include them because a) they are in {{input}} element and b) they might well be useful.

They are not removed, right?

@DanChadwick
Copy link
Contributor

Ah, I see. attrs.xxx was renamed passThru.xxx, as can be seen in the template. I misunderstood your comment on slack.

@DanChadwick
Copy link
Contributor

Other issues:

  1. I see that we are rendering rows="1" for <input ... elements. This is a regression caused by calling setupTextArea in handleInput. Suggest moving the test of whether the element is a textarea to into setupTextArea.
  2. When .md-input-invalid is added, it causes the placeholder to be displayed on top of the invalid text when the element is not focused.
  3. The default tabindex is -1. I think it should be 0. Or maybe there is something I don't understand?
  4. By eliminating our use of the TextSupport mixin, we also lost support for cut and paste events. What confuses me is that in my app where I'm using a contenteditable div, I'm not seeing input events for cut and paste. I can't reproduce this issue in the dummy app with FireFox on Windows, but the core team added this to TextSupport, which leads me to believe there was a reason. So I can't reproduce a bug, but this worries me a bit and I wonder if we shouldn't prophetically add cut and paste handlers? Or not? TextSupport also handles change events and I don't know why.

@miguelcobain
Copy link
Collaborator Author

I see that we are rendering rows="1" for <input ... elements. This is a regression caused by calling setupTextArea in handleInput. Suggest moving the test of whether the element is a textarea to into setupTextArea.

Good catch. Fixed.

When .md-input-invalid is added, it causes the placeholder to be displayed on top of the invalid text when the element is not focused.

Isn't this what is supposed to happen?
I'm not understanding the issue. 😞
I compared to AM demo. Couldn't spot a difference on that.

The default tabindex is -1. I think it should be 0. Or maybe there is something I don't understand?

Changed default to null. This way it is never added to the DOM. I can't remove it because it is something handled by BaseFocusable, and this is an exception to the general case.

By eliminating our use of the TextSupport mixin, we also lost support for cut and paste events. What confuses me is that in my app where I'm using a contenteditable div, I'm not seeing input events for cut and paste. I can't reproduce this issue in the dummy app with FireFox on Windows, but the core team added this to TextSupport, which leads me to believe there was a reason. So I can't reproduce a bug, but this worries me a bit and I wonder if we shouldn't prophetically add cut and paste handlers? Or not? TextSupport also handles change events and I don't know why.

We never supported those events on paper-input (cut and paste). The input event will trigger whenever the input changes through the user interface. onchange event only triggers on focus out, I think.

@DanChadwick
Copy link
Contributor

  1. Thx.
  2. Enter an invalid value. Placeholder is still shrunken and elevated. Leave element. Placeholder is now big and overlapping the invalid input.
  3. Is no tabindex treated as 0 in all browsers? If not, 0 would be better.
  4. We did have cut, paste and change support previously by way of TextSupport by way of {{input}}. What cases they handle, I'm not sure. Grow textarea maybe. Dunno. I

@miguelcobain
Copy link
Collaborator Author

Is no tabindex treated as 0 in all browsers? If not, 0 would be better.

I'm trying to replicate AM in this respect. No tabindex is used. I think 0 would not be the case because we don't want to focus <md-input-container, but the <input itself.

@miguelcobain
Copy link
Collaborator Author

Enter an invalid value. Placeholder is still shrunken and elevated. Leave element. Placeholder is now big and overlapping the invalid input.

Check me trying to replicate that:
http://webmshare.com/play/Gyz87

@miguelcobain
Copy link
Collaborator Author

We did have cut, paste and change support previously by way of TextSupport by way of {{input}}. What cases they handle, I'm not sure. Grow textarea maybe. Dunno. I

How did we support that? I don't see any reference to that in our codebase:
https://github.com/miguelcobain/ember-paper/search?utf8=%E2%9C%93&q=cut
https://github.com/miguelcobain/ember-paper/search?utf8=%E2%9C%93&q=paste

@DanChadwick
Copy link
Contributor

We "supported" cut etc by virtue of using core {{input}} which mixes in TextSuport, which supports it. See its init.

@DanChadwick
Copy link
Contributor

Confused. Our tabindex is on the input element, not the container (I think ... On phone atm). Just saying that a default of 0 would be nice. We don't have to mirror AM api. Because Ember. :)

@miguelcobain
Copy link
Collaborator Author

We "supported" cut etc by virtue of using core {{input}} which mixes in TextSuport, which supports it. See its init.

Nevermind that. That is probably a remnant of old browser support we needed back then.
Edit: it was used for 3rd party softwares like 1Password and TextExpander.

Confused. Our tabindex is on the input element, not the container (I think ... On phone atm). Just saying that a default of 0 would be nice. We don't have to mirror AM api. Because Ember. :)
We also had tabindex in the component itself set to -1. :)

<input elements are always focusable, so no need to add a tabindex.

@DanChadwick
Copy link
Contributor

Check me trying to replicate that:

Try Firefox, although it could be a FireFox / Windows thing. Enter a non-numeric value in a number field. Try the same in the Angular demo. The angular demo keeps the placeholder shrunken and elevated, even with abc in a number field, when you leave (i.e. blur) the field. We do not.

Looking at this, it's a mess. The <input> element has a value="" and textLength=0, despite having abc in the actual element. The validity.badInput=true. I'm not sure how cross-browser validity is though.I wonder what angular is doing to avoid this.

@miguelcobain miguelcobain merged commit 8e2c9d3 into wip/v1.0.0 Apr 12, 2016
@DanChadwick DanChadwick deleted the DanChadwick-paper-input branch April 15, 2016 18:49
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.

2 participants