fix(select): Do not fire change event on programmatic change #5255
Conversation
packages/mdc-select/component.ts
Outdated
| this.foundation_.setValue(value); | ||
| // Replicate behavior of native select, which does not emit change events | ||
| // when the value is changed programmatically. | ||
| this.foundation_.setValue(value, false /* notifyChange */); |
There was a problem hiding this comment.
Since setValue is used for programmatic change of select's value and I think we do not need notifyChange boolean.
Only handleChange() triggers event, where as setValue() does not.
WDYT?
There was a problem hiding this comment.
Good point. Removed the boolean from setValue so that it's only in handleChange.
Codecov Report
@@ Coverage Diff @@
## master #5255 +/- ##
=======================================
Coverage 98.63% 98.63%
=======================================
Files 122 122
Lines 5991 5991
Branches 770 768 -2
=======================================
Hits 5909 5909
Misses 81 81
Partials 1 1
Continue to review full report at Codecov.
|
|
All 731 screenshot tests passed for commit a98a20e vs. |
packages/mdc-select/foundation.ts
Outdated
| @@ -227,9 +230,8 @@ export class MDCSelectFoundation extends MDCFoundation<MDCSelectAdapter> { | |||
| /** | |||
| * Handles value changes, via change event or programmatic updates. | |||
| */ | |||
| handleChange() { | |||
| handleChange(notifyChange = true) { | |||
There was a problem hiding this comment.
We can remove boolean from here as well and make handleChange always notify the change.
We'll need to abstract out the code from handleChange to private method so it can be reused in other places (setValue, setSelectedIndex, etc). It is a good practice to bind handle* methods to only event callbacks in component implementation so we won't break other calls when we introduce any changes to handleChange callback in future.
Currently handleChange is called in component implementation and also used has helper function within foundation.
There was a problem hiding this comment.
Updated. setValue and setSelectedIndex now both won't fire a change event since they are both programmatic changes.
|
Please let me know when this PR is ready for review. |
|
All 731 screenshot tests passed for commit 92f3893 vs. |
Fixes #5234
Changes to
layout-gridandtextfieldare purely to pass the linter.