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

fix(buttons): make buttons selectors more specific #1678

Closed
wants to merge 1 commit into from
Closed

fix(buttons): make buttons selectors more specific #1678

wants to merge 1 commit into from

Conversation

pkozlowski-opensource
Copy link
Member

@pkozlowski-opensource pkozlowski-opensource commented Jul 12, 2017

BREAKING CHANGE:

Selectors for radio buttons related directives were changed and now both label
and input require ng-bootstrap specific attributes as selectors.

Before:

<div [(ngModel)]="model" ngbRadioGroup>
  <label class="btn">
    <input type="radio" name="radio" [value]="values[0]"/> {{ values[0] }}
  </label>
  <label class="btn">
    <input type="radio" name="radio" [value]="values[1]"/> {{ values[1] }}
  </label>
</div>

After:

<div [(ngModel)]="model" ngbRadioGroup>
  <label ngbButtonLabel>
    <input ngbButton type="radio" name="radio" [value]="values[0]"/> {{ values[0] }}
  </label>
  <label ngbButtonLabel>
    <input ngbButton type="radio" name="radio" [value]="values[1]"/> {{ values[1] }}
  </label>
</div>

Notice new ngbButtonLabel and ngbButton attributes that act as new selectors.

Fixes #1125

Copy link
Member

@maxokorokov maxokorokov left a comment

Choose a reason for hiding this comment

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

LGTM, two things:

  1. Also could have named them NgbRadioGroupNgbRadioLabelNgbRadio

  2. Will you update demos separately?

@@ -57,8 +57,8 @@ export class NgbRadioGroup implements ControlValueAccessor {
}


@Directive({selector: 'label.btn'})
export class NgbActiveLabel {
@Directive({selector: '[ngbButtonLabel]', host: {'[class.btn]': 'true'}})
Copy link
Member

Choose a reason for hiding this comment

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

class: btn

Copy link
Member Author

Choose a reason for hiding this comment

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

This would potentially overwrite classes that people would like to set on the <label> element, so I would prefer to leave it as-is.

BREAKING CHANGE:

Selectors for radio buttons related directives were changed and now both label
and input require ng-bootstrap specific attributes as selectors.

Before:

```
<div [(ngModel)]="model" ngbRadioGroup>
  <label class="btn">
    <input type="radio" name="radio" [value]="values[0]"/> {{ values[0] }}
  </label>
  <label class="btn">
    <input type="radio" name="radio" [value]="values[1]"/> {{ values[1] }}
  </label>
</div>
```

After:

```
<div [(ngModel)]="model" ngbRadioGroup>
  <label ngbButtonLabel>
    <input ngbButton type="radio" name="radio" [value]="values[0]"/> {{ values[0] }}
  </label>
  <label ngbButtonLabel>
    <input ngbButton type="radio" name="radio" [value]="values[1]"/> {{ values[1] }}
  </label>
</div>
```

Notice new `ngbButtonLabel` and `ngbButton` attributes that act as new selectors.

Fixes #1125
@pkozlowski-opensource
Copy link
Member Author

@maxokorokov thnx for the review - I've completely forgotten about the demo part!!! Pushed amended commit with the demo.

Regarding renames - I'm going to open another PR with the checkbox buttons where (hopefully) the existing names will make more sense. Hold a sec.

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

2 participants