Fix selected attribute on <wa-option> when <wa-select> has with-clear #1922#1985
Fix selected attribute on <wa-option> when <wa-select> has with-clear #1922#1985kelseythejackson merged 11 commits intonextfrom
<wa-option> when <wa-select> has with-clear #1922#1985Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
I can't replicate this outside of the repro provided in #1922, which uses Vue. I'm going to request that @KonnorRogers also take a look at this, since he has intricate knowledge about form-association and framework bindings. I'm mostly concerned about the proposed changes having negative effects outside of the framework. |
KonnorRogers
left a comment
There was a problem hiding this comment.
Form association stuff should be fine, im a little concerned about having a decent number of options slowing down the DOM constantly re-running the selectionUpdated on <wa-select> / <wa-combobox>
| protected firstUpdated(changedProperties: PropertyValues<this>) { | ||
| super.firstUpdated(changedProperties); | ||
|
|
||
| // If the `selected` property was set directly (e.g., by Vue's :selected binding), | ||
| // notify the parent select to update its selection. This is needed because | ||
| // Vue binds to the `selected` property instead of the `defaultSelected` property | ||
| // when using `:selected="true"` syntax. | ||
| if (this.selected && !this.defaultSelected) { | ||
| const parent = this.closest<WaSelect>('wa-select, wa-combobox'); | ||
| if (parent && !parent.hasInteracted) { | ||
| parent.selectionChanged?.(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🤔 I'm a little worried about this firing many times for initial page load as selectionChanged() is going to iterate all <wa-option> elements everytime a new one is added and could make this fire many times, and to me seems like an expensive operation.
There was a problem hiding this comment.
Any suggestions to move this forward?
There was a problem hiding this comment.
I think I'd feel safer with something like a debounced selectionChanged tbh, but thats just my personal preference.
This is probably fine tbh
|
I requeued the failing tests. There's a chance the timeout was just a hiccup. |
<wa-option> when <wa-select> has with-clear #1922
|
The test still fails: https://github.com/shoelace-style/webawesome/actions/runs/21646558998/job/62484414100?pr=1985 Looking again, it seems legitimate. |
|
@KonnorRogers Don't we expect this to work in Vue et al? (Note the use of <wa-select label="selected" :value="option-4">
<wa-option value="option-1">Option 1</wa-option>
<wa-option value="option-2">Option 2</wa-option>
<wa-option value="option-3">Option 3</wa-option>
<wa-option value="option-4">Option 4</wa-option>
</wa-select>Aren't the |
|
Correct, direct value setting is supported and should work. The only place it gets weird is setting a value attribute, as we only support that for selecting a single item. |
|
Original Reproduction: https://stackblitz.com/edit/vitejs-vite-n4i4ys4n?file=src%2FApp.vue With Kelsey's fix: https://stackblitz.com/edit/vitejs-vite-fftfnzsv?file=src%2FApp.vue,src%2Fmain.js it does seem to fix the broken selects. |
KonnorRogers
left a comment
There was a problem hiding this comment.
Just add a changelog entry, and i think this is good to go.
…e-style/webawesome into kj/bug-bash-with-clear-wa-select
Issue
Fixes #1922
wa-select-wa-option: Selected attribute ignored on wa-option when wa-select has with-clearThe selected attribute on wa-option elements was being ignored when the parent wa-select had the with-clear attribute.
Root Cause
The issue was in wa-option's willUpdate lifecycle method. On first render, it was unconditionally setting selected = defaultSelected (which defaults to false), overwriting any selected value that was set directly by frameworks like Vue.
Changes Made
src/components/option/option.tsselectedwhen it was set directly by frameworks.:selected="true"binding).src/components/select/select.test.tsAdded three tests to verify the fix:
selectedattribute withwith-clearselectedattribute,with-clear, andplaceholderselectedattribute andwith-clearTest Results
All tests in the select component pass.
Other Components
wa-inputalso useswith-clearbut is not affected - it doesn't use child elements with selected attributes