feat(svelte-ds-app-launchpad): upstream Link & Breadcrumbs components#438
feat(svelte-ds-app-launchpad): upstream Link & Breadcrumbs components#438
Conversation
Upstream the Link component from launchpad-ui with semantic tokens and separated CSS. Also fix component tokens in styles/index.css.
Upstream the Breadcrumbs component from launchpad-ui with semantic tokens, separated CSS, and sub-components (CollapsedItems, ExpandedItems, Item). WebKit focus tests marked as todo due to macOS TabsToLinks preference.
There was a problem hiding this comment.
Pull request overview
This PR upstreams the Link and Breadcrumbs components to the ds-app-launchpad package. These components were generated using @canonical/summon:component-svelte v0.1.0 and follow established patterns in the codebase. The Link component is a styled anchor element with an optional soft variant that inherits the parent's color. The Breadcrumbs component is a navigation component with intelligent collapsing behavior when space is limited, featuring progressive enhancement for non-JavaScript environments.
Changes:
- Added Link component with support for soft styling and comprehensive test coverage
- Added Breadcrumbs component with smart collapsing mechanism and accessibility features
- Updated CSS variables from
--dimension-*to--lp-dimension-*prefix for consistency
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/svelte/ds-app-launchpad/src/lib/styles/index.css | Updated CSS variable names to use --lp- prefix for consistency |
| packages/svelte/ds-app-launchpad/src/lib/components/index.ts | Added exports for Link and Breadcrumbs components |
| packages/svelte/ds-app-launchpad/src/lib/components/Link/* | New Link component with types, styles, tests, and stories |
| packages/svelte/ds-app-launchpad/src/lib/components/Breadcrumbs/* | New Breadcrumbs component with collapsing logic, sub-components, comprehensive tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/svelte/ds-app-launchpad/src/lib/components/Breadcrumbs/common/ExpandedItems/styles.css
Outdated
Show resolved
Hide resolved
...ages/svelte/ds-app-launchpad/src/lib/components/Breadcrumbs/common/CollapsedItems/styles.css
Outdated
Show resolved
Hide resolved
packages/svelte/ds-app-launchpad/src/lib/components/Breadcrumbs/Breadcrumbs.stories.svelte
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/svelte/ds-app-launchpad/src/lib/components/Link/Link.stories.svelte
Show resolved
Hide resolved
packages/svelte/ds-app-launchpad/src/lib/components/Link/Link.svelte.test.ts
Show resolved
Hide resolved
packages/svelte/ds-app-launchpad/src/lib/components/Link/Link.ssr.test.ts
Show resolved
Hide resolved
packages/svelte/ds-app-launchpad/src/lib/components/Breadcrumbs/types.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
You have two nested common folders, I am unsure this is intentional
There was a problem hiding this comment.
yes since Item is only consumed by parent level CollaspedItems and ExpandedItems it is by definition a common component for these two components and it should be placed inside a nested common folder.
...te/ds-app-launchpad/src/lib/components/Breadcrumbs/common/ExpandedItems/ExpandedItems.svelte
Outdated
Show resolved
Hide resolved
advl
left a comment
There was a problem hiding this comment.
I left a couple comments passing by - I would also recuse myself from a full review here as I'm not a svelte specialist.
I would nevertheless recommend strongly to split the PR into two (or do this for future ones) - as smaller PRs are easier - and faster to review.
This is likely something to document in the issues template as well.
@advl I agree with you 100% but I would say this PR fits into a medium sized PR rather than big. 1200 lines added is not a lot to review in one PR. But if we check by number of files changed, 30 files is a lot. However 30 files for 5 total components (3 sub components) and all of them are fully connected since each component requires at least 4 files it adds up (index, styles, stories, ssr test, client test, svelte file). |
|
@goulinkh Yes ! however I would add small size beats medium size - there are companies where this advice is applied as a principle and really help smoothen the dev process - I'd be keen to discuss this more. In all cases it would be great to set the expectations on this together for the monorepo |
jmuzina
left a comment
There was a problem hiding this comment.
Reviewed the rest of it - looking good. Just leaving some non-blocking comments. Thanks!
| collapseClickOpened = !collapseClickOpened; | ||
| }} | ||
| > | ||
| <ol role="none" data-testid="collapsed-segments"> |
There was a problem hiding this comment.
We shouldn't need to hard code data-testid inside the core implementation of the component
There was a problem hiding this comment.
Not sure what a good alternative would be here, though, as this is a roleless element, which does not provide a way to pass arbitrary props (can't pass testid from the test), and selecting it is heavily relied on for auto-collapsing tests.
| const maxNumCollapsed = $derived( | ||
| minNumExpanded === "all" | ||
| ? 0 | ||
| : Math.max(0, segments.length - Math.max(0, minNumExpanded)), | ||
| ); | ||
| let containerWidth = $state<number>(); | ||
| let segmentWidths = $state<number[]>([]); |
There was a problem hiding this comment.
This auto-collapsing/expanding behavior is really really impressive and intuitive..... first implementation of this new requirement for Breadcrumbs. Great work LP folks!!
| collapseClickOpened = !collapseClickOpened; | ||
| }} | ||
| > | ||
| <ol role="none" data-testid="collapsed-segments"> |
There was a problem hiding this comment.
Just curious, why role="none"? Likely worth a comment at least
| type LinkSegment = Omit<LinkProps, "children" | "href"> & { | ||
| label: string; | ||
| href: string; | ||
| }; |
There was a problem hiding this comment.
Why Omit href if you're going to add it back in the type here? Is there some type incompatibility?
There was a problem hiding this comment.
To make sure it is required for the LinkSegment as this is eventually used to differentiate between the members of the Segment discriminated union (in the Item component).
macOS WebKit only tabs to form controls by default; Option+Tab is needed to include links. Extract shared tabToNextFocusable helper into src/lib/test-utils.ts so other components can reuse it.

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
Breadcrumbs