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

NgbModule.forRoot() breaks 'traditional' style radio buttons #1125

Closed
GeorgeA93 opened this issue Dec 8, 2016 · 16 comments
Closed

NgbModule.forRoot() breaks 'traditional' style radio buttons #1125

GeorgeA93 opened this issue Dec 8, 2016 · 16 comments

Comments

@GeorgeA93
Copy link

Bug description:

I am attempting to use 'traditional' style radio buttons, i.e the radio's you can see here

The plunker below shows that these radio's do not work when NgbModule.forRoot() has been called.

You can still interact with the radios, however they do not show the initial value of the model.

Link to minimally-working plunker that reproduces the issue: http://plnkr.co/edit/6BTqmJESmtsjWGwgUuo0?p=preview

Version of Angular, ng-bootstrap, and Bootstrap:

Angular: 2.0.0

ng-bootstrap: 1.0.0-alpha.14

Bootstrap: 4.0.0-alpha.5

@alex321
Copy link
Contributor

alex321 commented Dec 8, 2016

This is caused by the fact that ngbRadioGroup is the one notifying all the inputs about the change. There is no ngbRadioGroup when you use default radios hence it breaks the functionality.

@pkozlowski-opensource I think we should introduce a selector [ngbRadioButton] for that instead of using input[type=button] or only apply it to descendants of ngbRadioGroup as it doesn't make sense to override default angular functionality. It's a deal breaker for the whole library if it interferes with standard behaviours. Could we patch that soon as it seems necessary and potentially look into incorporating #1095 ?

@pkozlowski-opensource
Copy link
Member

@alex321 you are right, we should probably have a more fine-grained selector... I'm going to look into this issue and your PR tomorrow.

@alex321
Copy link
Contributor

alex321 commented Dec 20, 2016

@pkozlowski-opensource I had a look around and as far as I am aware, we are doing what we should be. Do you think that this might be related to angular in any way?

Here's a brief summary that explains my confusion (I am basing it on GeorgeA93's plunker):
I changed the selector for the buttons from input[type=radio] to ngbButton so that I leave Angular to handle the default radios. Then I used choiceControl.registerOnChange but that didn't fire when I change either the default buttons or the ngbButton.

However, if I use choiceControl.setValue(), it does fire as expected. And then both buttons respond as expected. Hence, I am confused how the two should know that a change has occurred.

@maxisam
Copy link

maxisam commented Feb 9, 2017

This is a nasty one. I spent 2 days for this, since I didn't know it is possible. I am on alpha.20. The standard behavior still doesn't work.

I love this library, but I think if we find a component that can break the default usage, we should remove it from the whole package temporary. Btw, It is not a BAN to that component. lol

ref: qdouble/angular-webpack-starter#247

@bprgit
Copy link

bprgit commented Feb 22, 2017

We have also spent some time with this issue.

Is there a work around for this issue and/or an ETA on when a fix may be available?

@maxisam
Copy link

maxisam commented Feb 22, 2017

My work around is pretty simple -- exclude ngbRadioButton component.

Just create a custom module to include everything but ngbRadioButton.

In fact, I think it might be a good idea to do this, just include the ones that you need.

Here it is, save you some time.

import {
    NgbAccordionModule,
    NgbAlertModule,
    // NgbButtonsModule,
    NgbCarouselModule,
    NgbCollapseModule,
    NgbDatepickerModule,
    NgbDropdownModule,
    NgbModalModule,
    NgbPaginationModule,
    NgbPopoverModule,
    NgbProgressbarModule,
    NgbRatingModule,
    NgbTabsetModule,
    NgbTimepickerModule,
    NgbTooltipModule,
    NgbTypeaheadModule
} from '@ng-bootstrap/ng-bootstrap';
import { ModuleWithProviders, NgModule } from '@angular/core';
const NGB_MODULES = [
    NgbAccordionModule, NgbAlertModule, NgbCarouselModule, NgbCollapseModule, NgbDatepickerModule,
    NgbDropdownModule, NgbModalModule, NgbPaginationModule, NgbPopoverModule, NgbProgressbarModule, NgbRatingModule,
    NgbTabsetModule, NgbTimepickerModule, NgbTooltipModule, NgbTypeaheadModule
];

@NgModule({
    imports: [
        NgbAlertModule.forRoot(), NgbCollapseModule.forRoot(), NgbProgressbarModule.forRoot(),
        NgbTooltipModule.forRoot(), NgbTypeaheadModule.forRoot(), NgbAccordionModule.forRoot(), NgbCarouselModule.forRoot(),
        NgbDatepickerModule.forRoot(), NgbDropdownModule.forRoot(), NgbModalModule.forRoot(), NgbPaginationModule.forRoot(),
        NgbPopoverModule.forRoot(), NgbProgressbarModule.forRoot(), NgbRatingModule.forRoot(), NgbTabsetModule.forRoot(),
        NgbTimepickerModule.forRoot(), NgbTooltipModule.forRoot()
    ],
    exports: NGB_MODULES
})
export class CNgbRootModule {
}

// tslint:disable-next-line:max-classes-per-file
@NgModule({ imports: NGB_MODULES, exports: NGB_MODULES })
export class CNgbModule {
    static forRoot(): ModuleWithProviders { return { ngModule: CNgbRootModule }; }
}

@bprgit
Copy link

bprgit commented Feb 24, 2017

Thanks, worked a treat.

@GoNode5
Copy link

GoNode5 commented Mar 6, 2017

@wesleycho
Copy link
Member

Looks like the culprit is that the provider needs to be provided into the module, not just the directive - see here for a working implementation.

I'll file a PR shortly.

@Ludevik
Copy link

Ludevik commented Jun 1, 2017

I have spent also quite some time with similar issue. In my case i tried to disable the form group containing the radio buttons. As soon as i import NgbModule, the disabling of the radio buttons breaks. When i remove the import, the radio buttons are disabled.

Plunk

The plunker shows also the problem with setting the value.

@changLiuUNSW
Copy link
Contributor

Really nasty! We have spent about 1 week and finally find out this culprit.
Is there any ETA for this issue?

@changLiuUNSW
Copy link
Contributor

changLiuUNSW commented Jun 15, 2017

@maxisam
I love this library, but I think if we find a component that can break the default usage, we should remove it from the whole package temporary. Btw, It is not a BAN to that component. lol
Totally agreed.
@pkozlowski-opensource Can we remove the whole component temporarily?

@pkozlowski-opensource
Copy link
Member

@changLiuUNSW we will remove default matching on radios before beta (hopefully or of 2-3 next releases). For now you can just skip importing this particular module in your app.

@jinwyp
Copy link

jinwyp commented Jun 30, 2017

..... I came here for setting default value for radio? Now that mean we need choose side for using angular4 or ng bootstrap ? LOL

@changLiuUNSW
Copy link
Contributor

@jinwyp So the whole component should be excluded until the issue is fixed

@mraiguo
Copy link

mraiguo commented Jul 7, 2017

when I update to 1.0.0-alpha.27,It was suddenly able to run, But not for a while 。I wanna know I , is this fixed?

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.