fix(rating): prevent rateChange to be fired when element is pristine #1306
Conversation
| @@ -85,6 +85,33 @@ describe('ngb-rating', () => { | |||
| expect(stars.length).toBe(3); | |||
| }); | |||
|
|
|||
| it('handles the change event', fakeAsync(() => { | |||
| const fixture = | |||
| createTestComponent('<ngb-rating [(rate)]="currentRate" (rateChange)="onRateChange($event)"></ngb-rating>'); | |||
There was a problem hiding this comment.
[(rate)]="currentRate" is a shortcut for [rate]="currentRate" (rateChange)="currentRate = $event". As such I don't think that we want to have 2 rateChange event handlers in this test.
More generally, IMO the simplest possible test for the issue we are trying to solve here could be:
it('should now fire change event initially', fakeAsync(() => {
const fixture = createTestComponent('<ngb-rating [rate]="3" (rateChange)="changed = true"></ngb-rating>');
tick();
expect(fixture.componentInstance.changed).toBeFalsy();
}));
|
@fbasso thnx for the PR - I left one comment about a test but it actually took me long time to understand the logic of the current rating control and the fix. When I finally got what is going on I think that we could a simpler fix. Your solution fixes the issue and does so in little code change which is good. But at the same time it introduces new state variable ( After going through the code of the rating directive I think that the main issue is that we are using the Given all the above I would like to propose a refactoring where we introduce explicit state for hovered value and don't change I've just opened #1357 with an alternative implementation - please have a look and let me know what you think and which one you would prefer to be merged. |
|
@pkozlowski-opensource Agree. I focused only on the issue but it's right that using the rate attribute for both current value and mouse over create a mess. The code you proposed render the rate component easier to understand (and fix this issue at the same time). So let's go for your changes ! |
Close #1282