angulardart / angular_components

The official Material Design components for AngularDart. Used at Google in production apps.
https://pub.dev/packages/angular_components
374 stars 123 forks source link

radio-group ngModel radio element not selected after change #61

Open chalin opened 7 years ago

chalin commented 7 years ago

The simple app from this repo runs as expected initially and displays this radio group with the currentHero selected:

screen shot 2017-03-14 at 10 12 04 am

This is an excerpt from the template used to generate the view:

    <material-radio-group [(ngModel)]="currentHero">
        <material-radio *ngFor="let h of heroes" [value]="h">
            {{h.name}} ({{h.id}})
        </material-radio>
    </material-radio-group>

But if you click "Reset Heroes", the current hero is no longer selected in the radio group:

screen shot 2017-03-14 at 10 12 17 am

The reset action code is:

  void resetHeroes() {
    heroes = Hero.mockHeroes.map((hero) => new Hero.copy(hero)).toList();
    currentHero = heroes[0];
  }

The full source for this demo (generated from a stagehand Angular web app base), is here. This app uses 3.0.0-alpha+1 version of Angular and the latest ACX.

This issue was originally discovered while working on the AngularDart 3.x sample for the Template Syntax page.

cc @kwalrath @filiph

chalin commented 7 years ago

@TedSander - oops, the code used to illustrate the issue wasn't using the proper version of ng and ACX. It is now.

chalin commented 7 years ago

@TedSander - I've added a vanilla ngDart version of the radio group to show that it works there but not in ACX. This is what the app looks like after a reset:

screen shot 2017-03-14 at 11 22 54 am

Notice that the vanilla ngDart radio group has Mr. Nice selected, but the ACX radio group has no selection.

This is the ngDart template HTML:

<p>Same functionality in vanilla AngularDart:</p>
<div *ngFor="let h of heroes">
    <input type="radio" name="heroes"
           [value]="h" [checked]="currentHero == h"
           (click)="currentHero = h">
    {{h.name}}
</div>
nshahan commented 7 years ago

Do you need to recreate the list of heros every time? I tried moving that code to ngOnInit and that works:

class AppComponent implements OnInit {
  List<Hero> heroes;
  Hero currentHero;

  void ngOnInit() {
    heroes = Hero.mockHeroes.map((hero) => new Hero.copy(hero)).toList();
    resetHeroes();
  }

  void resetHeroes() {
    currentHero = heroes[0];
  }
}
chalin commented 7 years ago

Yes, that is part of the original demo.

In fact, it is the first part of a section illustrating the use of ngFors trackby feature.

TedSander commented 7 years ago

I think I have a solution but I won't be able to implement it for awhile (going on vacation.) A workaround is to set the value on the next turn:

void resetHeroes() {
  heroes = Hero.mockHeroes.map((hero) => new Hero.copy(hero)).toList();
  Timer.run(() {currentHero = heroes[0];});
}

Another option to to set the checked value on the radio itself like you did with the plain value above.

Longer explanation: The code is here: https://github.com/dart-lang/angular2_components/blob/master/lib/src/components/material_radio/material_radio_group.dart#L199

If there are no children previous this works as it saves the value to check on any children that may come later, but since there are already children it checks the current children for the value. In this case it actually finds the value, but it will be destroyed/re-created. Even if it doesn't find the value the value will be dropped as invalid.

Separate note: one should be careful about recreating lists without using tracked by. Throwing away the views associated with those list items and recreating them is usually not what the developer wants, and it is unperformant. We've had quite a few bugs with engineers returning a different list and them expecting the view wouldn't change as much as it did.

chalin commented 7 years ago

Thanks. Looking forward to the complete solution.

FYI, Timer.run(() {currentHero = heroes[0];}); doesn't work. I've updated the sample code in case you'd like to try it out yourself.

Separate note: one should be careful about recreating lists without using tracked by.

Right. This code is atypical since it is meant to illustrate why users would want to use trackBy.