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

Added input error for required text field #120

Merged

Conversation

DanMonroe
Copy link
Contributor

No description provided.

@DanMonroe
Copy link
Contributor Author

This is my initial start to adding validation to the input fields. I'll be adding other stuff soon such as max characters, email validation, etc. Let me know if you'd like to change anything with my approach so far.

@miguelcobain
Copy link
Collaborator

Let me just give you some general advice, considering my experience in porting components from angular material and the direction of this project (especially #119).

  • Just copy and paste the input styles: input.scss and input-theme.scss
  • Stuff like {{foreground-3}} becomes color($foreground, '3')
  • Port input.js to an Ember component (sometimes more than one)
  • Inspect (input demos)[https://material.angularjs.org/latest/#/demo/material.components.input] to understand their DOM structure and come up with the templates

The thing is that the current ember text field has a lot of old stuff ported from polymer. We should try our best to align ourselves with angular material. That way we can get all the benefits from a larger community (and maybe give back something as well).

Love the tests.

@DanMonroe
Copy link
Contributor Author

Thanks for the comments Miguel. So is the goal to basically port the Angular implementation over to Ember? If so, what would you like to do with all the "ng-whatever" DOM elements and stylsheets?

For example, in Angular's DOM, they have ng-message attributes for each error.

<div ng-messages>
      <div ng-message="required">This is required.</div>
</div>

I used error-message instead of ng-message. I didn't know what to use instead of ng and thought ember-message wasn't descriptive enough. Basically, what would you like us to use instead of "ng" ?

Also, how much refactoring of the old polymer code would you like to see? I didn't want to change much for my first pull request; all my changes were additions to existing code.

Thanks Miquel. You've done a great job so far and I hope I can help check things off your todo list.

@miguelcobain
Copy link
Collaborator

If their styles use ng-whatever, we should use it as well. I know I'm basically proposing to write paper-input from scratch, but I think that's something inevitable sooner or later.

From a first look, it looks like we may have some problems with animations because it looks like they're using ngAnimate.

If you have any problems, post here we'll certainly solve it together!

@DanMonroe
Copy link
Contributor Author

Ok, for this last commit, I basically took all of angular's input.scss and input-theme.scss and put it into your paper-text-scss. I had to change md-input-group to md-input-container to match the css.

I'm playing with the transition for the error text.

@DanMonroe
Copy link
Contributor Author

I've added the rest of the validation and also the tab fix that you asked for. The only thing missing (from the angular implementation) is the animation. I'm not going to be able to work on it this week because of my real job. Are you thinking of merging it into master?

@miguelcobain
Copy link
Collaborator

@DanMonroe No problem. I'll try to have a look at the animation issue if I have the time.

This will be merged into master when it is complete. Good job.

@DanMonroe
Copy link
Contributor Author

I've added fade in and fade out animation for the errors. I created a separate animation.scss file that currently has my two animations, however, we can add a lot more animations. These were based off of http://daneden.github.io/animate.css/

@miguelcobain
Copy link
Collaborator

No success in using the existing angular material classes for animation?
If we don't use them, we'll need to maintain our own.

Angular uses ngAnimate. Ideally we should come up with a mixin that mimics the necessary ngAnimate's functionality, and reuse it for other components.

@cah-danmonroe
Copy link
Contributor

Ok, I'll try to use the ngAngular animations during lunch today.

@cah-danmonroe
Copy link
Contributor

I've added ng-animation.scss for use throughout the project. I've updated paper-text to use the ng animations.

@miguelcobain
Copy link
Collaborator

Can you elaborate?

Why do we need the ng-animation.scss? The idea was to leave the styles untouched and use the existing animations, but maybe I'm missing something.

@cah-danmonroe
Copy link
Contributor

You're right. I didn't see the existing transforms when I searched earlier. I removed ng-animation.scss and I believe that everything is in place now.

@miguelcobain
Copy link
Collaborator

@DanMonroe THIS IS RAD!

Awesome job.

I'm just getting an error with sass:

File: ember-paper/tmp/tree_merger-tmp_dest_dir-eXxYnFvM.tmp/paper-text.scss (263)
invalid selector after ,

Corresponds to this line:

md-input-container.md-#{$theme-name}-theme {
  .md-input {
    &::-webkit-input-placeholder, /* <----- 263 is here */
    &::-moz-placeholder, /* Firefox 19+ */
    &:-moz-placeholder, /* Firefox 18- */
    &:-ms-input-placeholder {
      color: color($foreground, '3');
    }

Even the syntax highlight is a bit messed up. The sass compiles without any problems at your side?

I deleted that stuff to check the changes, and it's really good. Got some "bugs" because I deleted the problematic rule, but apart from that, it's looking great!

Also, I think we should drop any md- prefixes when passing component variables.
But I can clean that up for you, no problem.

I think there's missing a md-minlength-errortext in the last example.

@DanMonroe
Copy link
Contributor Author

My ember build works. I don't know, maybe it was the comments it didn't like. I removed them just in case.

The md- prefix was just brought in to match what the material.angularjs.org was doing. Go ahead and remove them if you think that reads better.

I didn't put in the md-minlength-errortext on purpose to demonstrate that there are default error messages when the -errortext suffixes were not used. Not sure how you want to communicate the parameters and the defaults in the example pages.

OK, so now I'm really done with this pull request unless you think there is anything else you want me to do. :)

@miguelcobain miguelcobain merged commit 34a6f44 into adopted-ember-addons:master Jul 9, 2015
@miguelcobain
Copy link
Collaborator

Tremendous job, @DanMonroe. Thank you.
Merged and published.

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

Successfully merging this pull request may close these issues.

3 participants