Conversation
There was a problem hiding this comment.
Pull request overview
Upstreams new form input components into the ds-app-launchpad Svelte design system package, adding a TextInput wrapper, a Textarea with optional dynamic row sizing, and a shared TextInputPrimitive, along with Storybook stories and Vitest coverage.
Changes:
- Export
TextInput/Textareafrom the public components barrel andTextInputPrimitivefromcomponents/common. - Add
TextInputPrimitive,TextInput, andTextareaimplementations + styles + Storybook stories. - Add client + SSR tests for the new components, and unit tests for
calculateDynamicRows; update packagetestscript to run the additional Vitest project.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/svelte/ds-app-launchpad/src/lib/components/index.ts | Re-exports TextInput and Textarea from the package component barrel. |
| packages/svelte/ds-app-launchpad/src/lib/components/common/index.ts | Re-exports TextInputPrimitive from the common components barrel. |
| packages/svelte/ds-app-launchpad/src/lib/components/common/TextInputPrimitive/types.ts | Defines prop typing for the input primitive. |
| packages/svelte/ds-app-launchpad/src/lib/components/common/TextInputPrimitive/styles.css | Base styling for the input primitive. |
| packages/svelte/ds-app-launchpad/src/lib/components/common/TextInputPrimitive/index.ts | Public entrypoint for TextInputPrimitive. |
| packages/svelte/ds-app-launchpad/src/lib/components/common/TextInputPrimitive/TextInputPrimitive.svelte.test.ts | Browser tests for the input primitive. |
| packages/svelte/ds-app-launchpad/src/lib/components/common/TextInputPrimitive/TextInputPrimitive.svelte | Implements the primitive input (bindable value/ref). |
| packages/svelte/ds-app-launchpad/src/lib/components/common/TextInputPrimitive/TextInputPrimitive.stories.svelte | Storybook stories for the input primitive. |
| packages/svelte/ds-app-launchpad/src/lib/components/common/TextInputPrimitive/TextInputPrimitive.ssr.test.ts | SSR tests for the input primitive. |
| packages/svelte/ds-app-launchpad/src/lib/components/Textarea/utils/index.ts | Barrel export for textarea utilities. |
| packages/svelte/ds-app-launchpad/src/lib/components/Textarea/utils/calculateDynamicRows.ts | Adds dynamic row calculation helper for textarea. |
| packages/svelte/ds-app-launchpad/src/lib/components/Textarea/utils/calculateDynamicRows.test.ts | Unit tests for the dynamic rows helper. |
| packages/svelte/ds-app-launchpad/src/lib/components/Textarea/types.ts | Defines Textarea props including static/dynamic rows. |
| packages/svelte/ds-app-launchpad/src/lib/components/Textarea/styles.css | Styling for Textarea. |
| packages/svelte/ds-app-launchpad/src/lib/components/Textarea/index.ts | Public entrypoint for Textarea. |
| packages/svelte/ds-app-launchpad/src/lib/components/Textarea/Textarea.svelte.test.ts | Browser tests for Textarea (incl. dynamic rows behavior). |
| packages/svelte/ds-app-launchpad/src/lib/components/Textarea/Textarea.svelte | Implements Textarea with derived rows behavior and bindable value/ref. |
| packages/svelte/ds-app-launchpad/src/lib/components/Textarea/Textarea.stories.svelte | Storybook stories for Textarea (static + dynamic rows). |
| packages/svelte/ds-app-launchpad/src/lib/components/Textarea/Textarea.ssr.test.ts | SSR tests for Textarea. |
| packages/svelte/ds-app-launchpad/src/lib/components/TextInput/types.ts | Defines TextInput props including severity/density modifiers. |
| packages/svelte/ds-app-launchpad/src/lib/components/TextInput/styles.css | Styling overrides for TextInput on top of the primitive. |
| packages/svelte/ds-app-launchpad/src/lib/components/TextInput/index.ts | Public entrypoint for TextInput. |
| packages/svelte/ds-app-launchpad/src/lib/components/TextInput/TextInput.svelte.test.ts | Browser tests for TextInput. |
| packages/svelte/ds-app-launchpad/src/lib/components/TextInput/TextInput.svelte | Implements TextInput as a styled wrapper around TextInputPrimitive. |
| packages/svelte/ds-app-launchpad/src/lib/components/TextInput/TextInput.stories.svelte | Storybook stories for TextInput incl. severities. |
| packages/svelte/ds-app-launchpad/src/lib/components/TextInput/TextInput.ssr.test.ts | SSR tests for TextInput. |
| packages/svelte/ds-app-launchpad/package.json | Updates the test script to run server tests in addition to ssr. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| */ | ||
| value?: string | number; | ||
|
|
||
| density?: "dense" | "medium"; |
There was a problem hiding this comment.
density is typed as "dense" | "medium", but this package’s modifier-families define a compact density as well. Since the component forwards density directly as a class (and modifier-families/styles/density.css contains .compact), consumers should be able to pass "compact" too. Consider typing this via ModifierFamily<['density','severity']> (or ModifierFamilyValues['density']) to stay consistent with other components (e.g. Chip/Button) and avoid drifting values.
| density?: "dense" | "medium"; | |
| density?: "compact" | "dense" | "medium"; |
| const linesCount = countLinesToLimit( | ||
| textareaValue, | ||
| Math.max(minRows, maxRows), | ||
| ); | ||
|
|
||
| if (linesCount <= minRows) { | ||
| return minRows; | ||
| } else if (linesCount <= maxRows) { | ||
| return linesCount; | ||
| } else { | ||
| return maxRows; |
There was a problem hiding this comment.
calculateDynamicRows can behave incorrectly when minRows > maxRows (which isn’t prevented by the rows tuple type). In that case linesCount is computed against Math.max(minRows, maxRows) but the function may still return the smaller maxRows, causing rows to shrink unexpectedly. Consider normalizing/validating the inputs (e.g., swap values when out of order or throw) and using the validated maxRows as the counting limit.
| const linesCount = countLinesToLimit( | |
| textareaValue, | |
| Math.max(minRows, maxRows), | |
| ); | |
| if (linesCount <= minRows) { | |
| return minRows; | |
| } else if (linesCount <= maxRows) { | |
| return linesCount; | |
| } else { | |
| return maxRows; | |
| const normalizedMinRows = Math.min(minRows, maxRows); | |
| const normalizedMaxRows = Math.max(minRows, maxRows); | |
| const linesCount = countLinesToLimit( | |
| textareaValue, | |
| normalizedMaxRows, | |
| ); | |
| if (linesCount <= normalizedMinRows) { | |
| return normalizedMinRows; | |
| } else if (linesCount <= normalizedMaxRows) { | |
| return linesCount; | |
| } else { | |
| return normalizedMaxRows; |
There was a problem hiding this comment.
This makes sense, I'd consider throwing n error when minRows > maxRows
There was a problem hiding this comment.
I have added a check to swap values in case the couple values are not in the proper order.
| export interface TextInputPrimitiveProps | ||
| extends Omit<HTMLInputAttributes, "children"> { | ||
| ref?: HTMLInputElement; | ||
| type?: "text" | "password" | "email" | "url" | "tel" | "search"; |
There was a problem hiding this comment.
Is type necessary here? Isn't it already part of HTMLInputAttributes?
There was a problem hiding this comment.
This has been moved to InputPrimitive, type excludes number. I have rewritten this is a more readable way.
There was a problem hiding this comment.
There's only story coverage for type="password", it would be good to cover the other possible cases (either in a "types" story or in one story per type, like you've done with password
There was a problem hiding this comment.
add some other stories in components/TextInput
| /** | ||
| * The type of input control to display. | ||
| */ | ||
| type?: "text" | "password" | "email" | "url" | "tel" | "search"; |
There was a problem hiding this comment.
Like my other comment, can't we use type from HTMLInputAttributes without adding it explicitly here?
There was a problem hiding this comment.
done in InputPrimitive
| import type { HTMLInputAttributes } from "svelte/elements"; | ||
| import type { ModifierFamily } from "modifier-families"; | ||
|
|
||
| export interface TextInputProps |
There was a problem hiding this comment.
TextInput spreads its props onto TextInputPrimitive, but there is no explicit relationship between their props. I think TextInputProps should extend TextInputPrimitiveProps - otherwise it is much easier for props we don't expect to be passed to one or both of these components.
|
I would like to attract your attention that it is becoming important to document the code standards for svelte going forward. This will facilitate reviews and shipping code. |
6f97944 to
2602023
Compare
|
Hi @jmuzina, I have updated the PR addressing your comments.
Sorry for missing this with the initial PR draft! |
|
I will let @steciuk take it from here, since I will be changing my focus next pulse on other topics. |

Fixes LP-3722
PR readiness check
Feature 🎁,Breaking Change 💣,Bug 🐛,Documentation 📝,Maintenance 🔨.package.json:check,check:fix, andtest.buildto build the package for development or distribution,build:allto build all artifacts. See CONTRIBUTING.md for details.Screenshots