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

Finished Paper Form Component #408

Merged
merged 22 commits into from
Jul 10, 2016

Conversation

shoxter
Copy link
Contributor

@shoxter shoxter commented Jun 29, 2016

Usage like this:

{{#paper-form parentSubmit=(action 'useless') as |form|}}

         {{form.input value=foo onChange=(action (mut foo)) label="Foo" required=true}}
         {{form.input value=bar onChange=(action (mut bar)) label="Bar"}}

         {{#paper-button primary=true raised=true onClick=form.submit disabled=form.isInvalid}}Button{{/paper-button}}

{{/paper-form}}

Different Options:

  • form.input: Paper inputs inside of the form.
  • parentSubmit: The action in the parent component/controller to execute (clear fields, create/save records, etc) with regards to the form prior to the form component resetting the isTouched property on all of the inputs.
  • form.submit: This should be the onClick method for the intended submit button for the form.
  • form.isInvalid: This is the property that the form changes as the form's inputs change their validity state. When the inputs are invalid, this will be true and should disabled the form's intended submit button.

The form looks at all the form.inputs with input constraints for invalidity.

Please post any questions. We can create a page on the dummy app to cover the documentation for this component once we cover any questions that should be included.

@shoxter shoxter closed this Jun 29, 2016
@shoxter shoxter reopened this Jun 29, 2016
@miguelcobain
Copy link
Collaborator

Should I consider this PR or #406 ?

@shoxter
Copy link
Contributor Author

shoxter commented Jul 4, 2016

@miguelcobain Both of them.

Multiple input validation in a form setting wasn't possible without the fix to inputs in #406.

I first had to fix the inputs to not send separate isInvalid calls for both 'NULL' and 'TRUE' values.

It was effectively double registering the invalid state of the input. #406 fixes that and this is the form component.

@DanChadwick
Copy link
Contributor

Maybe we should have isValad so null is better aligned with false.

@@ -4,7 +4,7 @@ export default Ember.Component.extend({
tagName: '',
classNames: [],
attributeBindings: ['style'],
style: "width:100%",
style: 'width:100%',
numberOfInvalids: 0,
isValid: Ember.computed('numberOfInvalids', function() {
return this.get('numberOfInvalids') === 0 ? true : false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

return condition ? true : false; is equivalent to return condition;

@shoxter shoxter closed this Jul 4, 2016
@shoxter shoxter deleted the paper-form-component branch July 4, 2016 17:31
@shoxter shoxter reopened this Jul 5, 2016
isValid: Ember.computed('numberOfInvalids', function() {
return this.get('numberOfInvalids') === 0;
}),
isInvalid: Ember.computed('isValid', function() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

destructure computed

@miguelcobain
Copy link
Collaborator

can't you work around the touchedTrigger by using _isTouched instead of isTouched on the templates?

@shoxter
Copy link
Contributor Author

shoxter commented Jul 8, 2016

@miguelcobain No because the value will never change in the parent component. The form is never going to set isTouched = true. So oldAttrs will always = newAttrs and will never execute. Unless I call a this.set('isTouched', true) then this.set('isTouched', false) which is more hacky than this solution.

The tests fail with that proposed solution (unless I do the true to false hack).

@miguelcobain
Copy link
Collaborator

@miguelcobain
Copy link
Collaborator

How do I reproduce?

@shoxter
Copy link
Contributor Author

shoxter commented Jul 8, 2016

@miguelcobain It didn't save for whatever reason.

This is the right one: https://ember-twiddle.com/d7bc618f48a24a89e1bd9411c0d49050/91f4de4c453bcbe8650134d9d72c026ead80cf45?openFiles=controllers.application.js%2C

1.) Toggle controller.
2.) Toggle component.
3.) Toggle controller again.

isValid: computed('childComponents.@each.isInvalid', function() {
return !this.get('childComponents').isAny('isInvalid');
}),
isInvalid: computed('isValid', function() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can use computed.not here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

isValid: computed.not('childComponents.@each.isInvalid', function() {
return this.get('childComponents').isAny('isInvalid');
}),
isInvalid: computed.not('isValid', function() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please read docs on computed.not. it doesn't take a callback

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry about that. I was in a rush out the door. I'll fix it when I get back.

@miguelcobain miguelcobain merged commit a9e236c into adopted-ember-addons:master Jul 10, 2016
@dustinfarris
Copy link
Contributor

If I want to use a custom paper-input component, is there an easy way to plug that in here?

@shoxter
Copy link
Contributor Author

shoxter commented Jul 11, 2016

{{#paper-form as |form|}}
  {{custom-paper-input parentComponent=form}}
{{/paper-form}}

@DanChadwick
Copy link
Contributor

@dustinfarris: I agree. Any component yielded should be created from an attribute. Contextual components tightly couple. 👎

@shoxter
Copy link
Contributor Author

shoxter commented Jul 11, 2016

@DanChadwick The paper-input is an attribute of the contextual component because it's the most common input yielded in the form.

It's impossible to take into account all possibilities in the contextual component (which I think is what you're saying), but you can make any paper-input based component a part of the form by adding parentComponent=form inside of the form component.

@dustinfarris
Copy link
Contributor

@shoxter thanks for that example. what if i'm using an intermediary component that proxies to paper-input? e.g. my usage here adopted-ember-addons/ember-changeset#59 (comment)

do i pass parentComponent down the chain? or just to the first component? is this officially supported?

@shoxter
Copy link
Contributor Author

shoxter commented Jul 11, 2016

@dustinfarris No problem.

The reason parentComponent=form works is because the form looks at all children components (components that you've specified that their parent is the form) for isInvalid.

isInvalid is a property of paper-input, so unless your proxy component has a property that aliases the isInvalid property of the paper-input in your template, you'll need to have it on the paper-input.

Make sense?

PS: Please note that the only reason parentComponent=form means anything and why the form can see it as a child is because of the mixin that we've extended into the paper-input.

@dustinfarris
Copy link
Contributor

@shoxter i think so, i'm just wondering how this will work with an intermediary component (i guess i'll find out soon enough!)

e.g.

my-route.hbs

{{#paper-form as |form|}}
  {{x-paper-input parentComponent=form}}
{{/paper-form}}

x-paper-input.hbs

{{paper-input parentComponent=parentComponent}}

@miguelcobain
Copy link
Collaborator

miguelcobain commented Jul 11, 2016

@dustinfarris x-paper-input extends paper-input?

Edit: i see it doesn't. If it did, there is a way to make this work without contextual components.

@miguelcobain
Copy link
Collaborator

@dustinfarris should be fixed with 35d457f

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.

4 participants