Skip to content

Conversation

@GenZodd
Copy link
Contributor

@GenZodd GenZodd commented Jun 13, 2017

Address issue #22

Tested this locally with the code base that triggered the creation of this issue. Works for template and reactive. Locally the tests will not run but they also do not run with the main GIT master branch so seems to be a local issue. Please review and provide any feedback.

@GenZodd
Copy link
Contributor Author

GenZodd commented Jun 21, 2017

@SethDavenport Not sure if you are the right person to bug but what needs to happen to get this reviewed and brought in?

@SethDavenport
Copy link
Member

SethDavenport commented Jun 21, 2017

Sorry for the delay on this - I actually haven't been super involved with angular-redux/form - I'm mostly the store and example-app guy. Unfortunately /form needs a new maintainer, and possibly a re-think, as you may have noticed.

My personal take is that we never intended to support the formBuilder stuff; in practice template-driven forms are much more declarative and therefore preferable.

@danielfigueiredo @clbond any thoughts on this change? It seems reasonable to me, but you have more experience with complex forms in Angular than I do.

}
else{
formElement = this.form.control;
}
Copy link
Member

Choose a reason for hiding this comment

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

formatting :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed up.


#### Reactive Forms
The value in "connect" attribute is the value that will show up in the Redux store. The formGrup value is the name of hte object in your code that represents the form group.

Copy link
Member

Choose a reason for hiding this comment

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

There are few typos 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.

Corrected.

source/module.ts Outdated
exports: [
Connect,
ConnectTemplateDirective,
ConnectReactiveDirective,
Copy link
Member

Choose a reason for hiding this comment

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

This represents a breaking API change - is there any way we can avoid this?

Copy link
Contributor Author

@GenZodd GenZodd Jun 22, 2017

Choose a reason for hiding this comment

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

I don't see how, as the source of the form has now changed between the two. Maybe we can keep the template class named "connect" so that class stays the same for people plugged in for template driven forms? Then, at least to me, the name of the class is less declarative on what it does but may be an option.

Choose a reason for hiding this comment

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

No need for a breaking change.

  • Connect and
  • ReactiveConnect
    are fine, too.

In @angular/forms its similar:

  • FormsModule
  • ReactiveFormsModule

Copy link
Member

Choose a reason for hiding this comment

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

yup I'm cool with that suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated PR based on this conversation to align.

@SethDavenport SethDavenport merged commit 6a75993 into angular-redux:master Jun 28, 2017
@SethDavenport
Copy link
Member

Give @angular-redux/form@6.5.0 a whirl and let me know.

@ghost
Copy link

ghost commented Nov 6, 2017

Still have never gotten this to work. :/ Not once have I even seen a form show up in my state. Are the docs up to date?

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