Skip to content

feat: add items prop, proper height and validation lib support to select #480

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 73 additions & 3 deletions packages/select/src/Select.svelte
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
<div
bind:this={element}
bind:clientWidth
use:Ripple={{
ripple: variant === 'filled',
unbounded: false,
Expand Down Expand Up @@ -50,13 +51,22 @@
>
{#if hiddenInput}
<input
type="hidden"
bind:this={input}
data-smui="true"
type="text"
style="width: 100%;height: 100%;position: absolute;opacity: 0;"
Comment on lines +54 to +57
Copy link
Owner

Choose a reason for hiding this comment

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

This will cause this input to be announced to screen readers.

{required}
{disabled}
{value}
{...prefixFilter($$restProps, 'input$')}
/>
{/if}
<style>
input[data-smui="true"]:focus-visible {
outline: none;
caret-color: transparent;
}
</style>
Comment on lines +64 to +69
Copy link
Owner

Choose a reason for hiding this comment

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

This will not work without "unsafe-inline" in the CSP.

<div
bind:this={selectAnchor}
use:useActions={anchor$use}
Expand Down Expand Up @@ -174,6 +184,7 @@
</div>

<Menu
style="min-width: {clientWidth ?? 0}px;"
Copy link
Owner

@hperrin hperrin Jul 22, 2022

Choose a reason for hiding this comment

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

This will be overwritten by the "list$style" prop. You would need to define, initialize, and incorporate that prop into here.

class={classMap({
[menu$class]: true,
'mdc-select__menu': true,
Expand All @@ -196,7 +207,17 @@
{wrapFocus}
bind:selectedIndex
on:SMUIList:mount={(event) => (list = event.detail)}
{...prefixFilter($$restProps, 'list$')}><slot /></List
{...prefixFilter($$restProps, 'list$')}>
{#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}
Comment on lines +211 to +219
Copy link
Owner

Choose a reason for hiding this comment

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

What is the reason for this instead of a slot?

</List
>
</Menu>
</div>
Expand All @@ -222,6 +243,7 @@
MDCSelectIconFoundation,
MDCSelectHelperTextFoundation,
} from '@material/select';
import Option from './Option.svelte';
import { MDCSelectFoundation } from '@material/select';
import { onMount, onDestroy, getContext, setContext } from 'svelte';
import { writable } from 'svelte/store';
Expand Down Expand Up @@ -250,6 +272,7 @@
import NotchedOutline from '@smui/notched-outline';

import HelperText from './helper-text/HelperText.svelte';
import Item from '@smui/list/src/Item.svelte';
Copy link
Owner

Choose a reason for hiding this comment

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

This is not how you import from other components. It would be:

import { Item } from '@smui/list';


const forwardEvents = forwardEventsBuilder(get_current_component());
interface UninitializedValue extends Function {}
Expand All @@ -258,6 +281,8 @@
return value === uninitializedValue;
}

export let input: any;
Copy link
Owner

@hperrin hperrin Jul 22, 2022

Choose a reason for hiding this comment

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

This needs to be more strict. "any" is not appropriate. Use HTMLInputElement.


// Remember to update types file if you add/remove/rename props.
export let use: ActionArray = [];
let className = '';
Expand Down Expand Up @@ -294,6 +319,9 @@
export let dropdownIcon$use: ActionArray = [];
export let dropdownIcon$class = '';
export let menu$class = '';
export let items: any[] = [];

let clientWidth;
Copy link
Owner

Choose a reason for hiding this comment

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

This needs a type.


let element: HTMLDivElement;
let instance: MDCSelectFoundation;
Expand All @@ -303,6 +331,37 @@
let selectAnchorAttrs: { [k: string]: string | undefined } = {};
let selectText: HTMLSpanElement;
let selectedIndex = -1;

// Set initial value on select that is already set (for form validation libs)
var didInputInitial = false;
$: if(input && !didInputInitial) {
didInputInitial = true;
value = input.value;
}
Comment on lines +336 to +340
Copy link
Owner

Choose a reason for hiding this comment

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

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)) {
Copy link
Owner

Choose a reason for hiding this comment

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

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.

// Reset select value
value = '';

// Set previous items for comparison
items_previous = items;

// Trigger events
dispatch(getElement(), 'SMUISelect:change', {
value,
index: selectedIndex,
}, undefined, true);
// Trigger event for hidden input, as well
if(input) {
dispatch(input, 'SMUISelect:change', {
value,
index: selectedIndex,
}, undefined, true);
}
}

let helperId: string | undefined = undefined;
let addLayoutListener = getContext<AddLayoutListener | undefined>(
'SMUI:addLayoutListener'
Expand Down Expand Up @@ -412,7 +471,10 @@
// Don't update the instance again.
previousSelectedIndex = index;
selectedIndex = index;
value = getMenuItemValues()[selectedIndex];

// Set the value without setting to "undefined"
var val = getMenuItemValues()[selectedIndex];
value = val ? val : '';
},
focusMenuItemAtIndex: (index) => {
list.focusItemAtIndex(index);
Expand Down Expand Up @@ -448,6 +510,14 @@
undefined,
true
);

// Trigger event for hidden input, as well
if(input) {
dispatch(input, 'SMUISelect:change', {
value,
index: selectedIndex,
}, undefined, true);
}
},

// getOutlineAdapterMethods
Expand Down