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

- add name to attribute bindings to specify a form element name #10

Merged
merged 3 commits into from
Apr 7, 2015

Conversation

enspandi
Copy link
Contributor

@enspandi enspandi commented Apr 2, 2015

No description provided.

@jonkoops
Copy link

jonkoops commented Apr 3, 2015

@enspandi Could you perhaps include the additional attributes discussed in #11 to your pull request?

@enspandi
Copy link
Contributor Author

enspandi commented Apr 4, 2015

Just added the remaining attributes: autofocus, form, required, size

@enspandi enspandi closed this Apr 4, 2015
@enspandi enspandi reopened this Apr 4, 2015
@jonkoops
Copy link

jonkoops commented Apr 4, 2015

Looks good to me. @cowboyd think we can get this merged?

@cowboyd
Copy link
Collaborator

cowboyd commented Apr 5, 2015

Looks great! Once we get some tests wrapped around this, merge is a go.

@enspandi
Copy link
Contributor Author

enspandi commented Apr 7, 2015

Sorry for the delay. I hope the tests are fine; wasn't sure to check for more than just the attribute binding. I appreciate any feedback!

@jonkoops
Copy link

jonkoops commented Apr 7, 2015

Unlike the multiple attribute these changes really don't add any new functionality to the internals. So I'd argue that these tests should be sufficient.

Nice work!

@cowboyd
Copy link
Collaborator

cowboyd commented Apr 7, 2015

Those tests were exactly what I was looking for. Mostly, I just want to prevent any accidental regressions on those attributes. 👍

cowboyd added a commit that referenced this pull request Apr 7, 2015
- add name to attribute bindings to specify a form element name
@cowboyd cowboyd merged commit c4b8225 into adopted-ember-addons:master Apr 7, 2015
@jonkoops
Copy link

jonkoops commented Apr 7, 2015

I'll go ahead and close #11, since this pull request fixes that issue.

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