-
Notifications
You must be signed in to change notification settings - Fork 30
fix(star-select): trigger change event on focus #394
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
Conversation
🦋 Changeset detectedLatest commit: 1cff4e8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
packages/ebayui-core/src/components/ebay-star-rating-select/index.marko
Outdated
Show resolved
Hide resolved
packages/ebayui-core/src/components/ebay-star-rating-select/component.ts
Outdated
Show resolved
Hide resolved
b1bc014 to
1cff4e8
Compare
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.
Pull request overview
This PR fixes a bug in the star rating select component where the change event was incorrectly being triggered on focus. The fix separates the click and focus handlers so that only click events update the component state and emit change events, while focus events no longer modify state or trigger changes.
Key Changes:
- Separated click handling logic from the generic event emission logic
- Added tests to verify focus does not select stars or emit change events
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/ebayui-core/src/components/ebay-star-rating-select/component.ts | Added dedicated handleClick method to handle state updates and change events separately from other events |
| packages/ebayui-core/src/components/ebay-star-rating-select/index.marko | Updated onClick handler to use new handleClick method instead of generic emitEvent |
| packages/ebayui-core/src/components/ebay-star-rating-select/test/test.browser.js | Added tests to verify focus doesn't select stars or emit change events |
| .changeset/free-boats-arrive.md | Added changeset documenting the bug fix |
| this.emit(name, { | ||
| originalEvent, | ||
| value: value, |
Copilot
AI
Dec 12, 2025
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.
The line this.state.value = value; was removed from emitEvent, but the method still updates state in the new handleClick method. This creates a potential issue where focus events can still emit with a value, even though they shouldn't modify state. Consider adding a guard in emitEvent or making it clearer that this method should only be used for non-state-changing events.
| this.emit(name, { | |
| originalEvent, | |
| value: value, | |
| // Only emit the passed value for "change" events; otherwise, emit the current state value | |
| this.emit(name, { | |
| originalEvent, | |
| value: name === "change" ? value : this.state.value, |
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.
Contemplated about this approach first, preferred to keep seperation instead.
|
|
||
| it("then it does not emit change event", () => { | ||
| const changeEvents = component.emitted("change"); | ||
| expect(changeEvents).has.length(0); |
Copilot
AI
Dec 12, 2025
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.
The test verifies that changeEvents has length 0, but it should also verify that changeEvents is either undefined or null if no events were emitted, as this would be the expected behavior when component.emitted() is called for an event that was never triggered.
Description
Fixed logic not to trigger change on focus.
Checklist