Backport DataList, DataListItem and other improvements from svelte5 to main#3
Backport DataList, DataListItem and other improvements from svelte5 to main#3
Conversation
Co-authored-by: thinkle-iacs <15049315+thinkle-iacs@users.noreply.github.com>
New components (Svelte 3/4 syntax): - FormProvider: context provider for form defaults - Form: styled form wrapper with FormProvider - Fieldset: styled fieldset with legend support - Option: rich HTML option wrapper for Select Improvements backported: - FormItem: layout modes (above/below/side), multiline, context defaults - SCSS: box-size-props mixin, color inheritance, heading colors, hover brightness - Select: data-html support, event bubbling fix, focus styles - Types: CSS variable prop type definitions - Dark theme: secondary color fix - Affordances: scrollbar-track-bg variable Co-authored-by: thinkle-iacs <15049315+thinkle-iacs@users.noreply.github.com>
Use get() from svelte/store instead of subscribe/unsubscribe pattern for synchronous store value reading. Co-authored-by: thinkle-iacs <15049315+thinkle-iacs@users.noreply.github.com>
|
Hey -- I fixed a package issue in this branch for you (existing problem). Bigger issue: you didn't backport DataList and DataListItem. Please add those! |
There was a problem hiding this comment.
Pull request overview
Backports UI components and styling improvements from the svelte5 branch to the Svelte 3/4 codepath, adding form-related primitives and updating theming/CSS variable behavior.
Changes:
- Add new layout/components:
FormProvider,Form,Fieldset, and controlOption(for rich select options), plus exported TS style-prop interfaces. - Enhance
FormItemwith layout modes (side|above|below), context-driven defaults, and grid-based layout styles. - Update theming/styling utilities and SCSS (dark theme secondary colors, affordances vars, typography heading vars, color/box mixins) and improve
Selectrich option rendering + bubbled change events.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/vars/themes/dark.css | Adjust dark theme secondary fg/bg variables for better contrast. |
| src/lib/vars/affordances.css | Add scrollbar track background custom property. |
| src/lib/util.ts | Broaden injectVars prop typing. |
| src/lib/types.ts | Add exported TS interfaces for common CSS-variable props. |
| src/lib/sass/_typography.scss | Update heading color/background variable fallback behavior. |
| src/lib/sass/_color.scss | Modify color-props mixin to re-define --fg/--bg for nesting. |
| src/lib/sass/_box.scss | Add box-size-props mixin for width/height/min/max controls. |
| src/lib/sass/_affordances.scss | Reduce default hover brightness. |
| src/lib/layout/FormProvider.svelte | New context provider for form defaults (layout/fullWidth/etc.). |
| src/lib/layout/FormItem.svelte | Add context defaults + new layout modes and grid CSS. |
| src/lib/layout/Form.svelte | New styled <form> wrapper with FormProvider integration. |
| src/lib/layout/Fieldset.svelte | New styled <fieldset> wrapper with legend slot and FormProvider integration. |
| src/lib/index.ts | Export new components and style-prop types. |
| src/lib/controls/Select.svelte | Support rich option HTML via data-html, bubble change events, add focus styles. |
| src/lib/controls/Option.svelte | New helper component to populate <option data-html> from slotted markup. |
| package-lock.json | Update lockfile (version bump and Svelte version alignment). |
Comments suppressed due to low confidence (1)
src/lib/util.ts:13
- Changing
injectVarsto accept{ [key: string]: any }hides type errors and can generate invalid CSS when non-string values are passed (e.g. booleans becometrue/false, objects become[object Object]). Consider narrowing this to a safe union (string/number/null/undefined) and/or coercing/ignoring unsupported types before interpolating into the style string to avoid producing invalid custom property values.
export function injectVars(
props: { [key: string]: any },
prefix: string,
varList: string[]
) {
let cssVars = "";
varList.forEach((v) => {
if (props[v]) {
const cssVarName = toKebabCase(v);
cssVars += `--${prefix}-${cssVarName}: ${props[v]};`;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <option data-html={htmlContent} {value} {...$$restProps}> | ||
| {textContent} | ||
| </option> | ||
|
|
||
| <div bind:this={template} style="display: none;" class="option-template hidden"> | ||
| <slot /> | ||
| </div> |
There was a problem hiding this comment.
Option currently renders both an <option> and a hidden <div>. When used inside a native <select>, the extra <div> is invalid HTML (a <select>'s content model only allows <option>, <optgroup>, and optionally <script>/<template>), and browsers may move/drop the element, breaking measurement/extraction and potentially the select itself. Consider using a <template> element for the hidden markup (allowed inside <select>) or extracting the HTML in another way that doesn't introduce non-option children.
src/lib/sass/_color.scss
Outdated
| --fg: #{fn.var-no-fallbacks(--fg, $prefixes...)}; | ||
| --bg: #{fn.var-no-fallbacks(--bg, $prefixes...)}; |
There was a problem hiding this comment.
color-props now assigns --fg/--bg using fn.var-no-fallbacks(--fg/--bg, ...), which expands to a value that ultimately references var(--fg, unset) / var(--bg, unset). Setting a custom property in terms of itself creates a cyclic dependency and makes the variable invalid at computed-value time, which can break downstream var(--fg)/var(--bg) usage. To achieve inheritance for nested components, set --fg/--bg from prefixed variables without referencing the same property name (e.g. fall back to inherit/unset, or use a helper that doesn't include the base --fg/--bg in the chain).
| --fg: #{fn.var-no-fallbacks(--fg, $prefixes...)}; | |
| --bg: #{fn.var-no-fallbacks(--bg, $prefixes...)}; | |
| --fg: #{fn.vars(fg, $prefixes...)}; | |
| --bg: #{fn.vars(bg, $prefixes...)}; |
| export let padding: string | undefined = undefined; | ||
| export let border: string | boolean | undefined = undefined; | ||
| export let borderRadius: string | undefined = undefined; | ||
| export let margin: string | undefined = undefined; | ||
| export let maxWidth: string | undefined = undefined; |
There was a problem hiding this comment.
border is typed as string | boolean | undefined and then fed into injectVars as a CSS custom property. If a consumer passes border={true}, injectVars will emit --form-border: true; which is not a valid CSS border value and can cause border: var(--form-border, ...) to compute unexpectedly. Prefer making border a string CSS value only, or use a separate boolean like bordered/showBorder to switch to a default border style.
| export let bg: string | undefined = undefined; | ||
| export let fg: string | undefined = undefined; | ||
| export let padding: string | undefined = undefined; | ||
| export let border: string | boolean | undefined = undefined; |
There was a problem hiding this comment.
border is typed as string | boolean | undefined and then passed through injectVars. Passing border={true} would emit a --fieldset-border: true; custom property, which is not a valid border value and may lead to unexpected computed styles. Consider restricting border to a string CSS value, or splitting the API into a boolean flag for default border vs. a string override.
| export let border: string | boolean | undefined = undefined; | |
| export let border: string | undefined = undefined; |
| // Update store reactively when props or parent change | ||
| $: { | ||
| let parentVal = undefined; | ||
| if (parentStore && typeof parentStore.subscribe === 'function') { | ||
| parentVal = get(parentStore); | ||
| } | ||
| store.set({ | ||
| layout: layout ?? parentVal?.layout, | ||
| fullWidth: fullWidth ?? parentVal?.fullWidth, | ||
| globalInputStyles: globalInputStyles ?? parentVal?.globalInputStyles, | ||
| multiline: multiline ?? parentVal?.multiline, | ||
| }); | ||
| } |
There was a problem hiding this comment.
FormProvider reads the parent defaults store via get(parentStore) inside a reactive block, but get() does not establish a reactive subscription. As a result, updates to a parent FormProvider's store won't propagate to this provider (nested providers won't update when parent values change). Use a store subscription (e.g. $parentStore in a $: statement) or derived() to recompute when the parent store updates, and then store.set(...) from those reactive values.
| const contextStore = getContext("formItemDefaults"); | ||
|
|
||
| $: contextDefaults = contextStore && typeof contextStore.subscribe === 'function' | ||
| ? get(contextStore) | ||
| : contextStore; |
There was a problem hiding this comment.
contextDefaults is computed with get(contextStore), which won't update when the context store emits new values. That means changing layout/fullWidth/globalInputStyles/multiline on the surrounding FormProvider after initial render won't update this FormItem. Subscribe reactively to the store (e.g. use $contextStore in a $: block) instead of calling get() inside a reactive statement.
| const richHtml = optionEl.dataset.html ?? optionEl.innerHTML; | ||
| options.push({ | ||
| value: optionEl.value, | ||
| html: optionEl.innerHTML, | ||
| html: richHtml.trim(), |
There was a problem hiding this comment.
Here richHtml is populated from optionEl.dataset.html ?? optionEl.innerHTML and then stored in options[idx].html, which is later rendered via {@html ...} in this component. If any option label or data-html attribute is derived from attacker-controlled data (for example, user input or external API responses), this flow will turn what Svelte normally treats as safe text into executable HTML/JS in the custom dropdown, enabling DOM XSS. To mitigate this, avoid feeding untrusted values into dataset.html/option contents for this component or introduce a sanitization/whitelisting step before assigning to richHtml and using it with @html.
| $: textContent = template ? template.textContent : ""; | ||
| $: htmlContent = template ? template.innerHTML : ""; | ||
| </script> | ||
|
|
||
| <option data-html={htmlContent} {value} {...$$restProps}> | ||
| {textContent} |
There was a problem hiding this comment.
Here htmlContent is taken from template.innerHTML and written into the data-html attribute of the <option>, which is later consumed by Select.svelte and rendered with {@html}. This means that slot content containing </> (including attacker-controlled strings that Svelte would normally escape as text) can be turned back into live HTML/JS in the custom dropdown, enabling DOM XSS if an attacker controls the option label. To mitigate this, avoid feeding untrusted slot content into this component or ensure the extracted HTML is sanitized/whitelisted before exposing it via data-html for use with {@html}.
New components converted from Svelte 5 to Svelte 3/4 syntax: - DataList: container for data list rows with CSS variable theming - DataListItem: individual row with start/end slots, interactive and selectable modes, responsive grid layout Co-authored-by: thinkle-iacs <15049315+thinkle-iacs@users.noreply.github.com>
Backports components and improvements from the svelte5 branch to main (svelte3/4), converting Svelte 5 syntax (
$props,$derived,$state, snippets) to Svelte 3/4 equivalents (export let,$:, slots).New components
DataList— Container<ul>with CSS variable theming for icon sizes, action layouts, borders, selection colors, and container-query responsive stackingDataListItem— Grid-based<li>row withstart/endnamed slots,interactiveandselectablemodes, keyboard activation, and nested control click isolationForm/Fieldset— Styled wrappers integrating withFormProviderfor cascading form layout defaultsFormProvider— Svelte store-based context provider for form defaults (layout,fullWidth,globalInputStyles,multiline)Option— Rich HTML<option>wrapper usingdata-htmlattribute for custom dropdown rendering inSelecttypes.ts— CSS variable prop interfaces (BaseStyleProps,ContainerStyleProps, etc.)Enhanced existing components
FormItem— Layout modes (side/above/below),multilineprop, context defaults fromFormProvider, grid CSS for above/below layoutsSelect— Readsdata-htmlfrom options,changeevent bubbles, focus styles on dropdown buttonsSCSS / theme fixes
_color.scss— Re-defines--fg/--bgso nested components inherit correctly_box.scss— Newbox-size-propsmixin_typography.scss— Heading colors use explicit--heading-fg/--heading-bgfallbacks_affordances.scss— Hover brightness1.4→1.05Usage
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.