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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Add input to ngbRadioGroup for allowing vertical group #1231

Closed
adamk33n3r opened this issue Jan 19, 2017 · 7 comments
Closed

Feature: Add input to ngbRadioGroup for allowing vertical group #1231

adamk33n3r opened this issue Jan 19, 2017 · 7 comments

Comments

@adamk33n3r
Copy link

It would be nice to specify whether we want it to be a .btn-group or a btn-vertical-group 馃槃

@pkozlowski-opensource
Copy link
Member

Why not. PRs welcomed - this should be rather simple change.

@dmytroyarmak
Copy link
Contributor

dmytroyarmak commented Jan 20, 2017

@pkozlowski-opensource What do you think would be better?

  1. Ask developer to specify class on [ngbRadioGroup] manually (don't set host's class on ngbRadioGroup element at all)
  2. Add parameter
    a. Add "orientation" parameter that should be "horizontal" (default) or "vertical"
    b. Add "vertical" parameter that should be true or false (default)

@pkozlowski-opensource
Copy link
Member

@dmytroyarmak my preference would go to (1) or (2a). Not sure which of the 2 is better.

Here are pros / cons for (1):

  • (+) don't need new API
  • (+) doesn't use binding
  • (-) requires more typing for the typical case
  • (-) breaking change (we don't care too much as this is still alpha)

If you take (2a) pros / cons are pretty much reversed as compared to (1)....

What would be your pick?

@dmytroyarmak
Copy link
Contributor

@pkozlowski-opensource
I think it's better to do (2a) to not introduce breaking change

@pkozlowski-opensource
Copy link
Member

I think it's better to do (2a) to not introduce breaking change

@dmytroyarmak let's do it! Fancy putting together a PR?

@dmytroyarmak
Copy link
Contributor

@pkozlowski-opensource yeah, I'll do a PR later today.

@pkozlowski-opensource
Copy link
Member

Update: I've decided to rather go with the option (1) from #1231 (comment)

In addition to the mentioned "pros" it will keep radio and checkboxes implementation consistent.

rmeans pushed a commit to fcsa-teamhammer/ng-bootstrap that referenced this issue Oct 18, 2017
Closes ng-bootstrap#1231
Closes ng-bootstrap#1238

BREAKING CHANGE:

The `btn-group` CSS class needs to added explicitly for radio buttons.

Before:

```
<div ngbRadioGroup ...>
  ...
</div>
```

After:

```
<div class="btn-group" ngbRadioGroup ...>
  ...
</div>
```

Closes ng-bootstrap#1733
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants