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

[FEATURE ember-handlebars-radio-buttons] Implement radio buttons #4352

Closed
wants to merge 1 commit into from
Closed

[FEATURE ember-handlebars-radio-buttons] Implement radio buttons #4352

wants to merge 1 commit into from

Conversation

ckornell
Copy link

This updates #1235 to extend component, adds handlebar helpers, updates tests and documentation. I think a lot of people would be pleased to see radio buttons in the core and I see a lot of developers at my company have a hard time with radio buttons when they first get started with Ember, so I think this would be a great addition to the core.

@mansona
Copy link
Member

mansona commented Feb 18, 2014

eh.. 👍 this looks fantastic! any comments from core on if this is likely to make it in soon?

@egaba
Copy link

egaba commented Feb 18, 2014

👍

@stefanpenner
Copy link
Member

@rjackson make sure this is on the next meeting schedule

@tlemens
Copy link

tlemens commented Mar 6, 2014

very useful

@nathanhammond
Copy link
Member

@stefanpenner @rjackson Is this one going to be addressed at EmberConf? Meeting minutes implied as much: http://emberjs.com/blog/2014/03/04/core-team-meeting-minutes-2014-02-28.html

@alexdiliberto and I will be using this for https://github.com/alexdiliberto/emberconf-2014-demo but we didn't want to go too far off the reservation...

alexdiliberto added a commit to alexdiliberto/emberconf-2014-demo that referenced this pull request Mar 16, 2014

```handlebars
{{#radiogroup}}
{{#radio value="admin"}}
Copy link
Member

Choose a reason for hiding this comment

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

This should not be a block element.

@nathanhammond
Copy link
Member

General

Underlying code works well, I'm pleased with it. I firmly believe that Ember should have an abstraction for all HTML input types–at this point I would say that new developers simply expect it because we've implemented it for a few of them. The fact that there are gaps amuses me.

jj-abrams-resolver (ember-cli)

Trying to load these in without using the global namespaces threw us for a loop. I'll defer that kind of thing to our resident experts. (@rjackson?)

API

I don't like the API differences between {{input type="checkbox"}} and {{radio}}. I feel like we should be consistent in our patterns. My preference is to "knock off" the HTML elements as it makes it more accessible to new Ember developers. It's also the pattern we follow everywhere else.

Along those lines, {{#radiogroup}} rubs me the wrong way. It's not a thing in HTML and forces adjacency (or huge blocks). My ideal API looks something like this:

{{input type="radio" name=role value="owner"}}
{{input type="radio" name=role value="group"}}
{{input type="radio" name=role value="everybody"}}

Underneath the hood it binds checked to the value of role and assigns the name property an arbitrary consistent value. Alternatively (note quotes):

{{input type="radio" name="role" value="owner" checked=role}}
{{input type="radio" name="role" value="group" checked=role}}
{{input type="radio" name="role" value="everybody" checked=role}}

Either one of these map more-closely to the HTML semantics while also feeling at home inside of Ember. The latter seems like it should work out of the box with minor modifications to Ember.checkbox.

Conclusion

I'm for inclusion of this functionality, but I want to revisit the API. We might also want to include this as part of a larger conversation on whether we want to move all form elements over to components instead of views.

@ckornell
Copy link
Author

Great thoughts, @nathanhammond. I would love to revisit the API with your suggestions once we hear the outcome of the core team's face-to-face conversation about this feature. I really like the simplicity of the latter option you presented; however, I have concerns about binding multiple instances of a component to one property.

It makes more sense to me to wait until the decision of the core team before discussing the API further. I look forward to future conversations on this topic and also your presentation at EmberConf! Good luck!

@wagenet wagenet added the feature label Apr 2, 2014
@wagenet
Copy link
Member

wagenet commented Apr 2, 2014

@cmkornell we should finally be discussing this on Friday. Can you rebase so it's ready if we decide to merge?

@ckornell
Copy link
Author

ckornell commented Apr 3, 2014

@wagenet Rebased.

@wagenet
Copy link
Member

wagenet commented Apr 3, 2014

@cmkornell looks like we're getting test failures now.

@ckornell
Copy link
Author

ckornell commented Apr 3, 2014

@wagenet :( I believe I have to ES6-ify this to get it back in good standing now. Looks like a lot has changed since I made the PR.

@wagenet
Copy link
Member

wagenet commented Apr 3, 2014

@cmkornell if you need a hand, let us know.

@ckornell
Copy link
Author

ckornell commented Apr 5, 2014

@wagenet I updated to ES6 and locally all tests are passing, but Travis is still angry with failing tests. I'm not quite sure what the issue is now. rake test shows me issues with just a clean remote master checked out, so I'm a bit confounded on the issues that still persist. Any insight would be greatly appreciated.

Also, any discussion on the inclusion of this feature today?

Thanks in advance.


selectedValueChanged: observer(function() {
var selectedValue = get(this, 'selectedValue');
if(!Ember.isEmpty(selectedValue) && get(this, 'value') === selectedValue) {
Copy link
Member

Choose a reason for hiding this comment

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

isEmpty needs to be imported from ember-metal/is_empty. (this is many of the failing tests)

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, @rjackson! Can you tell me why that didn't show up when I ran the Test Suite via rackup?

Copy link
Member

Choose a reason for hiding this comment

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

When you run all of the tests (depending on order), the actual exports run (therefore Ember.isEmpty does exist). When you run only this packages tests using local rackup with a url of http://localhost:9292/?package=ember-handlebars you would have gotten this error also.

rake test runs a number of different sets of QUnit URL params. Here is a list (possibly not complete):

  • Each packages tests.
  • Each packages tests with optional features enabled.
  • The final built output against all tests (actual confirming that the generated dist/ember.js file is usable).
  • The final built + minified output against all tests (to ensure that the minification does not break the tests).

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense. Thank you very much, @rjackson!

@ckornell
Copy link
Author

ckornell commented Apr 5, 2014

@wagenet All is well (finally). Please let me know what else I can do. :)

@rwjblue
Copy link
Member

rwjblue commented Apr 6, 2014

This was discussed in the 2014-04-04 core team meeting (see here).

Resolution

We would prefer for this to live as an addon for a while to allow the community to test (and influence) the API.

@rwjblue
Copy link
Member

rwjblue commented Apr 7, 2014

Going forward, we will be moving more things out of core that do not need access to internal private methods (this doesn't seem to need that), and into addons that can be pulled in as needed.

Closing this PR, but I would be happy to help set up a new project that could be pulled in via Bower.

@rwjblue rwjblue closed this Apr 7, 2014
@mansona
Copy link
Member

mansona commented Apr 7, 2014

@rjackson are there any guidelines for the "best" way to write these kinds of bower addons? Yes this particular PR would benefit from it but i think the community would too

@rwjblue
Copy link
Member

rwjblue commented Apr 7, 2014

@mansona - No nothing really exists guide wise at the moment. I'd be happy to help someone through the process with the intention of adding a guide once setup.

@digitalplaywright
Copy link

@rjackson @cmkornell Do you know if anyone is working on making this into an addon? I would like to use radio buttons in my design, and using this would be great since it seems to be the right way to implement radio buttons.

@nathanhammond
Copy link
Member

@digitalplaywright You can hop over to the files tab and figure out how to pull a fully-working version of this code into your application. The Ember addons story isn't well-defined yet, but once it is I'm sure that something like this will be amongst the first things to show up.

@izidorome
Copy link

we really need this...

@courajs
Copy link
Contributor

courajs commented May 18, 2014

@rizidoro I'm taking a stab now at creating @nathanhammond 's API as a helper available on bower. I assume @cmkornell is working on the same for his original API. The idea is they don't want it in core, so the community can settle on the right API for it

@courajs
Copy link
Contributor

courajs commented Jun 3, 2014

I took a stab at this. @nathanhammond's second proposal was simpler.
Just the helper: https://gist.github.com/FellowMD/7973c9bec27f0e0a3508
Project including the helper (uses ember-cli) https://github.com/FellowMD/ember-radio/tree/simpler

What I ended up with trying the second proposal is used like this:

{{radio-button checked='thing' value='one'}} One
{{radio-button checked='thing' value='two'}} Two
Currently selected: {{thing}}

I'm pretty interested in feedback on this. It seems too simple.
Just the helper: https://gist.github.com/FellowMD/84f5b113de2db762cb1d
Project including the helper https://github.com/FellowMD/ember-radio/tree/cooler

@rjackson It seems a lot easier at the moment to publish as a single script assuming a global ember namespace, à la Ember Table (http://addepar.github.io/#/ember-table/overview) Would that be the best thing to do at the moment?

@nathanhammond
Copy link
Member

@FellowMD I'll take a look at this soon. :)

@courajs
Copy link
Contributor

courajs commented Jun 10, 2014

bump - looking for a wee bit of feedback. Otherwise I guess I'll just publish it as a script which assumes global.

@blessanm86
Copy link

@FellowMD Im using ur radio button implementation and it works fine. But for some reason my observers only fire once. Im not sure if its an issue or just something wrong from my end. I've added a comment to ur gist. It would be great if you could look into it whenever u get time. https://gist.github.com/FellowMD/7973c9bec27f0e0a3508

@courajs
Copy link
Contributor

courajs commented Jun 27, 2014

@Blessenm Hey I was traveling. I couldn't reproduce, it works for me. If you'd still like help please open an issue on my repo https://github.com/fellowmd/ember-radio

@mansona
Copy link
Member

mansona commented Sep 3, 2014

Hey all! I just turned this into a ember-addon but i'm having some issues, maybe someone could take a look an see what they can do to help? https://github.com/Blooie/ember-radio-buttons

In it's current incarnation it's throwing an error Uncaught Error: Assertion Failed: registerBoundHelper-generated helpers do not support use with Handlebars blocks. but if i remove the Ember.Handlebars.makeBoundHelper() wrapping i don't get any access to the view.RadioButton any more.

I may just try to implement this again using @FellowMD's implementation but any help would be appreciated. If anyone wants to discuss this i'm real_ate in #emberjs and #ember-cli on freenode

@mansona
Copy link
Member

mansona commented Sep 3, 2014

... actually it turns out @FellowMD 's implementation works fine 👍 i understand your sentiment that it seems a bit simple but it works beautifully 😄 if you get a chance could you please check out the addon implementation and make sure you're happy with it. I've just tested it there and it's working fine

@pwfisher
Copy link

FellowMD's implementation is nice; I've built my solution based on it. I agree with nathanhammond that the interface should be similar to html and consistent with other Ember input components, i.e. similar to

{{ input type='text' value=value }}

The problem here is that where other inputs have a single value, the radio input has both an individual value and a group value. What value means was not clear enough for me in any of the other solutions I've seen, so I wrote my own:

{{ radio-button name='dish' value='spam' groupValue=selectedDish }}
{{ radio-button name='dish' value='eggs' groupValue=selectedDish }}

I've wrapped it up in a single file solution (!) -- a templateless component (though, as of now, we still need an empty template file).

@workmanw
Copy link

@rwjblue Does that mean we should expect {{input}} and {{textarea}} to be removed from Ember core at some point? I certainly appreciate not wanting to balloon the UI helper aspects of Ember. But it's awfully embarrassing to explain to people why you have to use a 3rd party library with ember for native HTML support.

@mmun
Copy link
Member

mmun commented Nov 14, 2014

@workmanw No.

@rwjblue
Copy link
Member

rwjblue commented Nov 14, 2014

I do not think {{input}} or {{textarea}} will be removed.

I talked a bit about this last night at Boston Ember (it was in the Q/A section near the end of the talk).

http://youtu.be/69wMY3CaklY?t=1h8m5s

@mmun
Copy link
Member

mmun commented Nov 14, 2014

They both do a bit of work (via TextSupport) to capture all the ways a input/textfield can be edited. This is useful for older browsers that don't support the change event.

Ideally in the future you can just use plain html elements:

<input value={{name}} on-change={{action 'updateName'}}>

@workmanw
Copy link

@mmun and @rwjblue Thanks for the responses!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.