feat: add items prop, proper height and validation lib support to select#480
feat: add items prop, proper height and validation lib support to select#480CryptAlchemy wants to merge 3 commits intohperrin:masterfrom
Conversation
hperrin
left a comment
hperrin
left a comment
There was a problem hiding this comment.
There are some issues that would need to be addressed before this approach would be acceptable. I also don't understand the need to use an items prop instead of a slot.
| bind:this={input} | ||
| data-smui="true" | ||
| type="text" | ||
| style="width: 100%;height: 100%;position: absolute;opacity: 0;" |
There was a problem hiding this comment.
This will cause this input to be announced to screen readers.
| <style> | ||
| input[data-smui="true"]:focus-visible { | ||
| outline: none; | ||
| caret-color: transparent; | ||
| } | ||
| </style> |
There was a problem hiding this comment.
This will not work without "unsafe-inline" in the CSP.
| </div> | ||
|
|
||
| <Menu | ||
| style="min-width: {clientWidth ?? 0}px;" |
There was a problem hiding this comment.
This will be overwritten by the "list$style" prop. You would need to define, initialize, and incorporate that prop into here.
| {#if items} | ||
| {#each items as item} | ||
| {#if item.name && item.value} | ||
| <Option value={item.value}>{item.name}</Option> | ||
| {:else} | ||
| <Option value={item}>{item}</Option> | ||
| {/if} | ||
| {/each} | ||
| {/if} |
There was a problem hiding this comment.
What is the reason for this instead of a slot?
| import NotchedOutline from '@smui/notched-outline'; | ||
|
|
||
| import HelperText from './helper-text/HelperText.svelte'; | ||
| import Item from '@smui/list/src/Item.svelte'; |
There was a problem hiding this comment.
This is not how you import from other components. It would be:
import { Item } from '@smui/list';| return value === uninitializedValue; | ||
| } | ||
|
|
||
| export let input: any; |
There was a problem hiding this comment.
This needs to be more strict. "any" is not appropriate. Use HTMLInputElement.
| export let menu$class = ''; | ||
| export let items: any[] = []; | ||
|
|
||
| let clientWidth; |
| var didInputInitial = false; | ||
| $: if(input && !didInputInitial) { | ||
| didInputInitial = true; | ||
| value = input.value; | ||
| } |
There was a problem hiding this comment.
This will force value to be a string no matter what type it is.
https://developer.mozilla.org/en-US/docs/Web/API/HTMLInputElement#value
|
|
||
| // If items array changes, reset value | ||
| let items_previous: any[] = []; | ||
| $: if (JSON.stringify(items) !== JSON.stringify(items_previous)) { |
There was a problem hiding this comment.
This causes potentially expensive JSON.stringify computations to run unnecessarily. It also means any change in items that has the same JSON representation wouldn't be picked up.
|
Using an items array limits the structure of whatever you pass into it, so I wouldn't be willing to do that. If you can make it just the height changes, I might could accept that. |
2 participants
Items are passed into the
Selectas either anobjector anarrayvia theitemsprop. Breaks existingSelectimplementations as there is no backwards-compatibility for in theSelect; anarrayorobjectmust be used. This deprecatesOption.svelte.The
Selectscales the menu according to device height ifmenu$fixed={true}prop is passed.SelectrespectshiddenInputprop by appropriately calling events on the field, including theSMUISelect:changeevent. ThehiddenInputalso contains adata-smui="true"prop for easy identification in standard browser-level form validation libraries.Closes #242, #468