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

feat(buttons): add support for checkbox buttons #1688

Closed
wants to merge 1 commit into from
Closed

feat(buttons): add support for checkbox buttons #1688

wants to merge 1 commit into from

Conversation

pkozlowski-opensource
Copy link
Member

Closes #890

@pkozlowski-opensource pkozlowski-opensource added this to the 1.0.0-alpha.29 milestone Jul 18, 2017
@pkozlowski-opensource pkozlowski-opensource changed the title WIP - feat(buttons): add support for checkbox buttons feat(buttons): add support for checkbox buttons Jul 19, 2017
@pkozlowski-opensource
Copy link
Member Author

@maxokorokov could you PTAL?

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, some minor comments.
Like the naming now 👍

checked;

/**
* A flag indicating if a given radio button is disabled.
Copy link
Member

Choose a reason for hiding this comment

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

radio button → checkbox


@Directive({selector: '[ngbButtonLabel]', host: {'[class.btn]': 'true'}})
export class NgbButtonLabel {
constructor(private _renderer: Renderer2, private _elRef: ElementRef) {}
Copy link
Member

Choose a reason for hiding this comment

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

True that we could have done it with 3 fields and host bindings, don't really need Renderer, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, will send a follow up PR to refactor it. Just didn't want to make too many changes in one PR.

* specified via ngModel.
*/
@Directive({
selector: '[ngbButton][type=checkbox]',
Copy link
Member

Choose a reason for hiding this comment

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

Ha, that's a much better solution!

src/index.ts Outdated
@@ -27,7 +27,7 @@ export {
NgbPanelContent
} from './accordion/accordion.module';
export {NgbAlertModule, NgbAlertConfig, NgbAlert} from './alert/alert.module';
export {NgbButtonsModule, NgbRadioGroup} from './buttons/radio.module';
export {NgbButtonsModule, NgbRadioGroup} from './buttons/buttons.module';
Copy link
Member

Choose a reason for hiding this comment

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

Seems strange, that we have only NgbRadioGroup here

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 is so people can query for it. We could add the new checkbox button here as well.

BREAKING CHANGE:

The `NgbButtonsModule` changed location (path) and content. This path
might need adjusting for people importing individual modules.
Before: `import {NgbButtonsModule} from './buttons/radio.module'`
After:  `import {NgbButtonsModule} from './buttons/buttons.module'`

The `NgbButtonsModule` now contains both checkbox and radio buttons.

Closes #890
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