Conversation
🦋 Changeset detectedLatest commit: 2687885 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for fondue-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for fondue-components ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
9a155cf to
9cf24ca
Compare
packages/rte/src/components/RichTextEditor/serializer/utils/serializeNodeToHtmlRecursive.ts
Fixed
Show fixed
Hide fixed
packages/rte/src/components/RichTextEditor/serializer/utils/serializeNodeToHtmlRecursive.ts
Fixed
Show fixed
Hide fixed
packages/rte/src/components/RichTextEditor/serializer/utils/serializeNodeToHtmlRecursive.ts
Fixed
Show fixed
Hide fixed
packages/rte/src/components/RichTextEditor/serializer/utils/serializeNodeToHtmlRecursive.ts
Fixed
Show fixed
Hide fixed
- Change default banner size from 'small' to 'large' - Extract state-dependent styles into card-state mixin for default, hover, selected, and hover+selected states - Rename grid area 'icon' to 'thumbnail' for shared logo/icon slot - Replace hardcoded sizes with sizeToken utility - Use transitions.transition-colors mixin for consistent animations - Fix focus ring to use :has(> .overlay:focus-visible) instead of :focus-visible on non-focusable root div - Update storybook with 280px wrapper and rename WithLargeBanner to WithSmallBanner to reflect new default Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update card styles to match Figma design tokens: replace hardcoded pixel values with sizeToken/spacing variables, use inset box-shadow for banner border, add surface-dim backgrounds, fix selection indicator states (hidden by default, visible on hover/selected), merge shared logo/icon styles, and add BannerFit story. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
9cf24ca to
370da1a
Compare
| tabIndex={0} | ||
| aria-label={ariaLabel} | ||
| aria-describedby={ariaDescribedby} | ||
| aria-current={selected ? 'true' : undefined} |
There was a problem hiding this comment.
is aria-current the right attribute for this? aria-selected might be a better choice
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Attributes/aria-current
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Attributes/aria-selected
There was a problem hiding this comment.
aria-selected isn't valid on link or button roles. We need an explicit role that supports it.
| onKeyDown={handleKeyDown} | ||
| /> | ||
| ) : ( | ||
| <button |
There was a problem hiding this comment.
This is the focusable element for the card. When it is focused by a screen reader there is no information about the content of the card at all.
Maybe we should add the `Card.Title as a sr-only text inside of it
There was a problem hiding this comment.
Great catch! Instead of duplicating the title as sr-only text inside the overlay (which could drift out of sync), we can use aria-labelledby to reference the Card.Title element directly. This way the overlay automatically picks up the title content as its accessible name, and if aria-label is explicitly provided it still takes precedence. The coordination between Card.Root and Card.Title will be handled via React useId()-generated id. What do you think?
packages/components/src/components/Card/styles/card.module.scss
Outdated
Show resolved
Hide resolved
|
Thank you very much for the PR! It's a great contribution! |
noahwaldner
left a comment
There was a problem hiding this comment.
The file is pretty lengthy, can we split it up into multiple comonents and hooks to improve readability?
| selected?: never; | ||
| }) | ||
| | { | ||
| href?: never; |
There was a problem hiding this comment.
why do we need this type that has never for all properties?
There was a problem hiding this comment.
That's the non-interactive variant of the union. It ensures TypeScript errors if you pass href, onSelect, selected, aria-label, or aria-describedby to a static (non-interactive) card.
Without it, a consumer could write <Card.Root onSelect={...}> (no href) and TypeScript wouldn't complain — the union would just match the second variant (CardRootInteractiveProps & { onSelect?: never }) and silently ignore the invalid combination.
The three variants enforce:
href+onSelect+selected— interactive & selectablehrefonly — interactive, not selectable- None of the above — static card, no interactive props allowed
It's a standard discriminated union pattern for mutually exclusive prop groups.
| <a | ||
| className={styles.overlay} | ||
| href={href} | ||
| tabIndex={0} |
There was a problem hiding this comment.
not needed as a <a> is focusable by default
There was a problem hiding this comment.
side note. does this work well with react router? ideally most of the time we'd probably want to use a Link component from react router?
There was a problem hiding this comment.
We have a <Link component in Fondue to be used instead of <a (we don't have dependencies on react-router)
There was a problem hiding this comment.
@SamuelAlev Should I add useFondueRouter() integration for client-side routing?
I believe we should not use the Link component here. The Card already handles its own styling, disabled states, and ARIA attributes. The Card overlay is an invisible, full-surface anchor (position: absolute; inset: 0) — it has no visible text, no underline, no font styling. All the visual props Link provides (size, weight, color, underline, wrap) are irrelevant.
There was a problem hiding this comment.
maybe just expose an onclick and a button not an a href?
There was a problem hiding this comment.
@mnoleto Sure, but keep the <a so hovering the card shows the link in the browser (also allow to drag in a new tab, cmb+click to open in new tab, ...)
Done! |
| navigate: () => { | ||
| throw new Error('RouterProvider: `navigate` function is not implemented'); | ||
| }, | ||
| navigate: () => {}, | ||
| // eslint-disable-next-line @eslint-react/no-unnecessary-use-prefix | ||
| useHref: () => { | ||
| throw new Error('RouterProvider: `useHref` function is not implemented'); | ||
| }, | ||
| useHref: (path: string) => path, |
There was a problem hiding this comment.
It is on purpose throwing an error, please keep it that way and add the provider in the renderer of Storybook/tests instead 🙏
There was a problem hiding this comment.
Reverted the changes
There was a problem hiding this comment.
@SamuelAlev Can we still use the useHref?
|
@mnoleto can you please add the missing translations in the other languages? |
|
Lead time: 44 days, 23 hours, 13 minutes, 43 seconds (1079.23 total hours) from first commit to close.
|

🎯 What does this PR do?
Card component
Introduces the Card compound component built on CSS Grid, following the Fondue compound component pattern.
Component structure
The component is not yet publicly exported and is marked as in_progress in Storybook.
TODO:
🖼️ Figma
https://www.figma.com/design/vYLXkUhWGHIpEftV1zTpqI/Patterns-v1.0?node-id=2123-5667&p=f&m=dev