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

feat(rating): enable form integration #1097

Merged
merged 1 commit into from
Dec 13, 2016

Conversation

maxokorokov
Copy link
Member

Enables form integration for rating:

  • both template driven and reactive forms tests
  • rate event emitter becomes asynchronous (same story as pagination)
  • ability to use both [(rate)] and [(ngModel)]
  • form integration demo with reactive

Fixes #1087

@@ -0,0 +1,16 @@
<p>You can use both ngModel and reactive forms. You don't have to use 'rate' binding in this case</p>
Copy link
Member

Choose a reason for hiding this comment

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

Probably would read better as

<p>ngModel and reactive forms can be used without using the 'rate' binding</p>

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Amended

@maxokorokov maxokorokov changed the title feat(rating): enable form intergation feat(rating): enable form integration Nov 30, 2016
@maxokorokov maxokorokov changed the title feat(rating): enable form integration feat(rating): enable form integration (DO NOT MERGE) Nov 30, 2016
@maxokorokov
Copy link
Member Author

Found an issue with readonly rating, will have to add some more tests...

@maxokorokov maxokorokov changed the title feat(rating): enable form integration (DO NOT MERGE) feat(rating): enable form integration Nov 30, 2016
@maxokorokov
Copy link
Member Author

It's OK now, but will add proper mouseenter and mouseleave tests in a separate PR, as we're missing them currently

@natcohen
Copy link

Any plan to merge this for the next release?

@pkozlowski-opensource
Copy link
Member

LGTM generally speaking. Could you just update the commit message to have reference to the issue this commit is fixing? Also this one has a breaking change, no? (sync -> async delivery of events?)

BREAKING CHANGES: event emitter behind the 'rateChange' output emits asynchronously now

Fixes ng-bootstrap#1087
@maxokorokov
Copy link
Member Author

Resolved conflicts and updated commit message with breaking changes. Can merge when green then.

@maxokorokov maxokorokov merged commit c090a5a into ng-bootstrap:master Dec 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants