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(module:rate): emit hover change when leave #8448

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Nicoss54
Copy link
Collaborator

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[X] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Application (the showcase website) / infrastructure changes
[ ] Other... Please describe:

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?

[ ] Yes
[X] No

Other information

Copy link

zorro-bot bot commented Mar 19, 2024

This preview will be available after the AzureCI is passed.

Copy link

codecov bot commented Mar 19, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 91.60%. Comparing base (116346f) to head (2ea061d).
Report is 15 commits behind head on master.

Files Patch % Lines
components/rate/rate.component.ts 83.33% 0 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

@HyperLife1119
Copy link
Collaborator

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.
Let's fix it here :)

onItemHover(index: number, isHalf: boolean): void {
if (this.nzDisabled || (this.hoverValue === index + 1 && isHalf === this.hasHalf)) {
return;
}
this.hoverValue = index + 1;
this.hasHalf = isHalf;
this.nzOnHoverChange.emit(this.hoverValue);
this.updateStarStyle();
}

@Nicoss54
Copy link
Collaborator Author

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. Let's fix it here :)

onItemHover(index: number, isHalf: boolean): void {
if (this.nzDisabled || (this.hoverValue === index + 1 && isHalf === this.hasHalf)) {
return;
}
this.hoverValue = index + 1;
this.hasHalf = isHalf;
this.nzOnHoverChange.emit(this.hoverValue);
this.updateStarStyle();
}

@HyperLife1119 it's done. Thanks again for the review

@@ -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);
Copy link
Collaborator

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

if (this.hoverValue === index + 1 && isHalf === this.hasHalf) {
return this.nzOnHoverChange.emit(this.hoverValue);
}

Copy link
Collaborator

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);
  }

Copy link
Collaborator Author

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 ?

Copy link
Collaborator

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?

Copy link
Collaborator Author

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

Copy link
Collaborator

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);
  }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@HyperLife1119 change done

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great job, Nicoss :)

Copy link
Collaborator

@HyperLife1119 HyperLife1119 left a comment

Choose a reason for hiding this comment

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

LGTM.
@OriginRing WDYT?

@Nicoss54 Nicoss54 force-pushed the feature/rate-emit-hover-change-when-leave branch from 4af62d1 to 2ea061d Compare April 5, 2024 15:33
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

3 participants