Skip to content

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

@zorro-bot
Copy link

zorro-bot bot commented Mar 19, 2024

This preview will be available after the AzureCI is passed.

@codecov
Copy link

codecov bot commented Mar 19, 2024

Codecov Report

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

Project coverage is 91.60%. Comparing base (116346f) to head (2ea061d).
Report is 78 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


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

@Nicoss54 Nicoss54 requested a review from HyperLife1119 March 20, 2024 12:30
if (this.hoverValue === index + 1 && isHalf === this.hasHalf) {
return this.nzOnHoverChange.emit(this.hoverValue);
}

Copy link
Collaborator

@HyperLife1119 HyperLife1119 Mar 21, 2024

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

@HyperLife1119 HyperLife1119 Mar 21, 2024

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
Copy link
Collaborator

@Laffery Laffery left a comment

Choose a reason for hiding this comment

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

LGTM

@Laffery Laffery merged commit 38dcc31 into NG-ZORRO:master Jun 4, 2024
@Nicoss54 Nicoss54 deleted the feature/rate-emit-hover-change-when-leave branch July 31, 2024 08:10
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.

4 participants