[BUGFIX beta] Add warning to variable {{input type}}, with working tests #5068

Merged
merged 1 commit into from Aug 4, 2014

Conversation

Projects
None yet
5 participants
@amiel
Contributor

amiel commented Jun 25, 2014

This is an update to #5056, which should fix the tests.

@stefanpenner

This comment has been minimized.

Show comment Hide comment
@stefanpenner

stefanpenner Jun 25, 2014

Member

so this isn't quite true, it is absolutely fine to bind to "type" for initial render, just subsequent changes are inert. This does make it tricky to warn people

Member

stefanpenner commented Jun 25, 2014

so this isn't quite true, it is absolutely fine to bind to "type" for initial render, just subsequent changes are inert. This does make it tricky to warn people

@amiel

This comment has been minimized.

Show comment Hide comment
@amiel

amiel Jun 25, 2014

Contributor

@stefanpenner Actually, I've found that it doesn't work if "type" is bound on initial render (in the case of checkboxes). See http://emberjs.jsbin.com/jezicuqe/4/edit for a failing example.

Contributor

amiel commented Jun 25, 2014

@stefanpenner Actually, I've found that it doesn't work if "type" is bound on initial render (in the case of checkboxes). See http://emberjs.jsbin.com/jezicuqe/4/edit for a failing example.

@amiel

This comment has been minimized.

Show comment Hide comment
@amiel

amiel Jun 25, 2014

Contributor

UPDATE: Added some comments to http://emberjs.jsbin.com/jezicuqe/9/edit

Contributor

amiel commented Jun 25, 2014

UPDATE: Added some comments to http://emberjs.jsbin.com/jezicuqe/9/edit

@stefanpenner

This comment has been minimized.

Show comment Hide comment
@stefanpenner

stefanpenner Jun 25, 2014

Member

i believe we should make it work dynamically for initial render, and error if it occurs on subsequent changes

Member

stefanpenner commented Jun 25, 2014

i believe we should make it work dynamically for initial render, and error if it occurs on subsequent changes

@amiel

This comment has been minimized.

Show comment Hide comment
@amiel

amiel Jun 25, 2014

Contributor

As discussed in irc, this should be a temporary solution.

Contributor

amiel commented Jun 25, 2014

As discussed in irc, this should be a temporary solution.

@stefanpenner

This comment has been minimized.

Show comment Hide comment
@stefanpenner

stefanpenner Jun 25, 2014

Member

relabel as bugfix beta

Member

stefanpenner commented Jun 25, 2014

relabel as bugfix beta

@amiel

This comment has been minimized.

Show comment Hide comment
@amiel

amiel Jun 25, 2014

Contributor

Done. Thanks!

Contributor

amiel commented Jun 25, 2014

Done. Thanks!

@amiel amiel changed the title from Add warning to variable {{input type}}, with working tests to [BUGFIX beta]Add warning to variable {{input type}}, with working tests Jun 26, 2014

@amiel amiel changed the title from [BUGFIX beta]Add warning to variable {{input type}}, with working tests to [BUGFIX beta] Add warning to variable {{input type}}, with working tests Jun 26, 2014

@amiel

This comment has been minimized.

Show comment Hide comment
@amiel

amiel Jun 26, 2014

Contributor

Ok, I added tests. They went in checkbox_test because that seemed like the most sensical place to me.

Contributor

amiel commented Jun 26, 2014

Ok, I added tests. They went in checkbox_test because that seemed like the most sensical place to me.

@amiel

This comment has been minimized.

Show comment Hide comment
@amiel

amiel Jul 11, 2014

Contributor

I've been thinking about trying to make the type work dynamically for checkboxes, and I think that fundamentally, it doesn't make sense for checkboxes to be handled by {{input}}. {{input type="checkbox"}} takes different options, and uses a different view. The fact that checkboxes are an <input> tag I think is irrelevant. <textarea> and <select> are semantically more similar to <input type="text"> than <input type="checkbox">.

I'm wondering if it would make sense, in the long run, to have {{checkbox}} instead of {{input type="checkbox"}}. I think this will also make more sense for {{radio}}.

I think, if a user legitimately wants to dynamically change the type, something like this makes more sense:

{{#if isCheckbox}}
  {{checkbox checked=value}}
{{else}}
  {{input type="text" value=value}}
{{/if}}

I know that we can't just take away {{input type="checkbox"}}, but we can add {{checkbox}}, and delegate {{input type="checkbox"}} for now.

Thoughts?

Contributor

amiel commented Jul 11, 2014

I've been thinking about trying to make the type work dynamically for checkboxes, and I think that fundamentally, it doesn't make sense for checkboxes to be handled by {{input}}. {{input type="checkbox"}} takes different options, and uses a different view. The fact that checkboxes are an <input> tag I think is irrelevant. <textarea> and <select> are semantically more similar to <input type="text"> than <input type="checkbox">.

I'm wondering if it would make sense, in the long run, to have {{checkbox}} instead of {{input type="checkbox"}}. I think this will also make more sense for {{radio}}.

I think, if a user legitimately wants to dynamically change the type, something like this makes more sense:

{{#if isCheckbox}}
  {{checkbox checked=value}}
{{else}}
  {{input type="text" value=value}}
{{/if}}

I know that we can't just take away {{input type="checkbox"}}, but we can add {{checkbox}}, and delegate {{input type="checkbox"}} for now.

Thoughts?

@trek

This comment has been minimized.

Show comment Hide comment
@trek

trek Jul 12, 2014

Member

This might fit oddly with HTMLBars/web components when we'll be able to do things like <my-foo> instead of {{my-foo}} and <input type="text" value=someBoundProp> instead of {{input type="text" value=someBoundProp}}.

People will expect handlebars <input> to have the same public API as bare HTML <input>

Member

trek commented Jul 12, 2014

This might fit oddly with HTMLBars/web components when we'll be able to do things like <my-foo> instead of {{my-foo}} and <input type="text" value=someBoundProp> instead of {{input type="text" value=someBoundProp}}.

People will expect handlebars <input> to have the same public API as bare HTML <input>

@amiel

This comment has been minimized.

Show comment Hide comment
@amiel

amiel Jul 14, 2014

Contributor

Hmmm, that's a good point. Clearly this needs some more discussion. Meanwhile this pull-request will hopefully prevent some confusion until we figure out how to deal with the {{input}} tag.

Is there anything else I need to do so this can get merged?

Contributor

amiel commented Jul 14, 2014

Hmmm, that's a good point. Clearly this needs some more discussion. Meanwhile this pull-request will hopefully prevent some confusion until we figure out how to deal with the {{input}} tag.

Is there anything else I need to do so this can get merged?

@@ -166,6 +166,8 @@ export function inputHelper(options) {
inputType = hash.type,
onEvent = hash.on;
+ Ember.assert('You can only use a string as a `type` parameter, not a variable', types.type === 'STRING' || types.type === undefined);

This comment has been minimized.

Show comment Hide comment
@rwjblue

rwjblue Jul 27, 2014

Member

Not sure this is quite right. It seems fine to me to use a bound property to look the type up, we just will not update upon that property changing.

@rwjblue

rwjblue Jul 27, 2014

Member

Not sure this is quite right. It seems fine to me to use a bound property to look the type up, we just will not update upon that property changing.

This comment has been minimized.

Show comment Hide comment
@amiel

amiel Jul 28, 2014

Contributor

@rwjblue Actually, I've found that to not be the case. The impetus for this PR was troubleshooting someone's issue on irc (#5047 (comment)) that was this case.

See http://emberjs.jsbin.com/jezicuqe/4/edit for a failing example. The checkbox actually does render, but it actually uses TextField to do it, which causes checked to not work.

@amiel

amiel Jul 28, 2014

Contributor

@rwjblue Actually, I've found that to not be the case. The impetus for this PR was troubleshooting someone's issue on irc (#5047 (comment)) that was this case.

See http://emberjs.jsbin.com/jezicuqe/4/edit for a failing example. The checkbox actually does render, but it actually uses TextField to do it, which causes checked to not work.

machty added a commit that referenced this pull request Aug 4, 2014

Merge pull request #5068 from amiel/input-type-nondynamic-warning
[BUGFIX beta] Add warning to variable {{input type}}, with working tests

@machty machty merged commit fc3799c into emberjs:master Aug 4, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@machty

This comment has been minimized.

Show comment Hide comment
@machty

machty Aug 4, 2014

Member

merging with the intent that we implement dynamically updating type in the future, but this is a good warning for now

Member

machty commented Aug 4, 2014

merging with the intent that we implement dynamically updating type in the future, but this is a good warning for now

@amiel

This comment has been minimized.

Show comment Hide comment
@amiel

amiel Aug 4, 2014

Contributor

@machty I started working on it, but need some guidance. I will ask on IRC

Contributor

amiel commented Aug 4, 2014

@machty I started working on it, but need some guidance. I will ask on IRC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment