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(module:rate): emit hover change when leave #8448
base: master
Are you sure you want to change the base?
feat(module:rate): emit hover change when leave #8448
Conversation
This preview will be available after the AzureCI is passed. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8448 +/- ##
==========================================
- Coverage 91.62% 91.60% -0.02%
==========================================
Files 530 531 +1
Lines 18390 18400 +10
Branches 2815 2814 -1
==========================================
+ Hits 16849 16855 +6
- Misses 1225 1228 +3
- Partials 316 317 +1 ☔ View full report in Codecov by Sentry. |
I think it's a bug that OnHoverChange event is not triggered when the rate value is unchanged. In this case, we still need to trigger OnHoverChange event. ng-zorro-antd/components/rate/rate.component.ts Lines 222 to 232 in 116346f
|
@HyperLife1119 it's done. Thanks again for the review |
components/rate/rate.component.ts
Outdated
@@ -221,6 +221,7 @@ export class NzRateComponent implements OnInit, ControlValueAccessor, OnChanges | |||
|
|||
onItemHover(index: number, isHalf: boolean): void { | |||
if (this.nzDisabled || (this.hoverValue === index + 1 && isHalf === this.hasHalf)) { | |||
this.nzOnHoverChange.emit(this.hoverValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the state is disabled, the event should not be triggered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
components/rate/rate.component.ts
Outdated
if (this.hoverValue === index + 1 && isHalf === this.hasHalf) { | ||
return this.nzOnHoverChange.emit(this.hoverValue); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the logic here would be this :) WDYT?
onItemHover(index: number, isHalf: boolean): void {
if (this.nzDisabled) {
return;
}
if (this.hoverValue !== index + 1 || isHalf !== this.hasHalf) {
this.hoverValue = index + 1;
this.hasHalf = isHalf;
this.updateStarStyle();
}
this.nzOnHoverChange.emit(this.hoverValue);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I'm not against this proposal quite the contrary :), but if we do that we break the current behavior of the component isn't it ? Maybe we could do a little RFC ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://stackblitz.com/edit/react-efsuqc?file=demo.tsx
Based on my observations of the behavior of the antd rate component, I think this is a bug fix.
If it is a breaking change, we need to merge it in the next major release.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HyperLife1119 after investigation on my side you're right, so i made the change :). Thanks for your time and help
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I think you're missing some details:
- When disabled, events are not triggered.
- The event still needs to be triggered even if the rate value has not changed.
I think it's supposed to work like this:
onItemHover(index: number, isHalf: boolean): void {
if (this.nzDisabled) {
return;
}
if (this.hoverValue !== index + 1 || isHalf !== this.hasHalf) {
this.hoverValue = index + 1;
this.hasHalf = isHalf;
this.updateStarStyle();
}
this.nzOnHoverChange.emit(this.hoverValue);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HyperLife1119 change done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job, Nicoss :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@OriginRing WDYT?
4af62d1
to
2ea061d
Compare
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Currently when the rate is left no event emitter is sent. This prevent customisation like changing icon
Issue Number: #8415
What is the new behavior?
hoverChange event is now sent when the user left the rate
Does this PR introduce a breaking change?
Other information