-
Notifications
You must be signed in to change notification settings - Fork 0
feat(ui-react/ui-rnative): use composite pattern for Subheader #398
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| <SubheaderTitle>Full Featured Subheader</SubheaderTitle> | ||
| <SubheaderCount value={42} /> | ||
| <SubheaderHint content={InfoTooltip} /> | ||
| <SubheaderAction onPress={() => console.log('Action clicked')}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[low]: onClick instead of onPress
| <TooltipContent>This is additional information</TooltipContent> | ||
| </Tooltip> | ||
| } /> | ||
| <SubheaderAction onPress={handleAction}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
| <SubheaderRow onPress={() => console.log('Row clicked')}> | ||
| <SubheaderTitle>Clickable Row</SubheaderTitle> | ||
| <SubheaderCount value={12} /> | ||
| </SubheaderRow> | ||
| <SubheaderDescription> | ||
| The entire row is clickable when onPress is provided | ||
| </SubheaderDescription> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
| <SubheaderRow onPress={handleClick}> | ||
| <SubheaderTitle>Clickable Row</SubheaderTitle> | ||
| <SubheaderCount value={12} /> | ||
| </SubheaderRow> | ||
| <SubheaderDescription> | ||
| The entire row is clickable when onPress is provided |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
| <Subheader> | ||
| <SubheaderRow> | ||
| <SubheaderTitle>Title</SubheaderTitle> | ||
| <SubheaderAction onPress={handlePress}>Action</SubheaderAction> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request refactors the Subheader component to use a composite component pattern, providing more flexibility in composing section headers. The changes affect both ui-react and ui-rnative libraries and introduce a new SectionHeader component in ui-react.
Key Changes:
- Converted Subheader from a prop-based API to a composite pattern with sub-components (SubheaderRow, SubheaderTitle, SubheaderCount, SubheaderHint, SubheaderDescription, SubheaderAction)
- Added comprehensive tests and storybook documentation for the new pattern
- Introduced a new SectionHeader component for ui-react (appears to be a different variant with different layout behavior)
- Updated sandbox app to demonstrate the new Subheader usage
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| libs/ui-rnative/src/lib/Components/index.ts | Adds Subheader to exports |
| libs/ui-rnative/src/lib/Components/Subheader/types.ts | Defines TypeScript types for composite Subheader components |
| libs/ui-rnative/src/lib/Components/Subheader/index.ts | Exports all Subheader components and types |
| libs/ui-rnative/src/lib/Components/Subheader/Subheader.tsx | Implements composite Subheader components for React Native |
| libs/ui-rnative/src/lib/Components/Subheader/Subheader.test.tsx | Comprehensive test suite for React Native Subheader |
| libs/ui-rnative/src/lib/Components/Subheader/Subheader.stories.tsx | Storybook stories demonstrating React Native usage |
| libs/ui-rnative/src/lib/Components/Subheader/Subheader.mdx | Documentation for React Native Subheader (contains errors) |
| libs/ui-react/src/lib/Components/Subheader/types.ts | Updated types for composite pattern in React |
| libs/ui-react/src/lib/Components/Subheader/index.ts | Updated exports for React Subheader components |
| libs/ui-react/src/lib/Components/Subheader/Subheader.tsx | Implements composite Subheader components for React |
| libs/ui-react/src/lib/Components/Subheader/Subheader.test.tsx | Updated test suite for React Subheader |
| libs/ui-react/src/lib/Components/Subheader/Subheader.stories.tsx | Updated storybook stories for React |
| libs/ui-react/src/lib/Components/Subheader/Subheader.mdx | Updated documentation for React Subheader |
| libs/ui-react/src/lib/Components/Subheader/Subheader.figma.tsx | Updated Figma integration (has missing prop) |
| libs/ui-react/src/lib/Components/SectionHeader/types.ts | New component type definitions |
| libs/ui-react/src/lib/Components/SectionHeader/SectionHeader.tsx | New SectionHeader component implementation (not exported) |
| libs/ui-react/src/lib/Components/SectionHeader/SectionHeader.stories.tsx | Storybook stories for new SectionHeader |
| apps/app-sandbox-rnative/src/app/blocks/index.ts | Adds Subheaders to sandbox exports |
| apps/app-sandbox-rnative/src/app/blocks/Subheaders.tsx | Sandbox examples for Subheader usage |
| apps/app-sandbox-rnative/src/app/App.tsx | Integrates Subheader examples in sandbox app |
Comments suppressed due to low confidence (1)
libs/ui-react/src/lib/Components/SectionHeader/SectionHeader.tsx:192
- The SectionHeader component is missing an index.ts file for exporting the component and types. All other components in the library follow the pattern of having an index.ts file that exports the component and types (e.g., Subheader/index.ts). This file should be created to maintain consistency with the codebase structure.
import { cn } from '@ledgerhq/lumen-utils-shared';
import React from 'react';
import {
SectionHeaderActionProps,
SectionHeaderCountProps,
SectionHeaderDescriptionProps,
SectionHeaderHintProps,
SectionHeaderProps,
SectionHeaderRowProps,
SectionHeaderTitleProps,
} from './types';
/**
* Container for the SectionHeader row that aligns title, count, hint, and action.
*/
export const SectionHeaderRow = React.forwardRef<
HTMLDivElement,
SectionHeaderRowProps
>(({ children, className, ...props }, ref) => {
return (
<div
ref={ref}
className={cn('flex items-center justify-between gap-8', className)}
{...props}
>
<div className='flex min-w-0 flex-1 items-center gap-6'>{children}</div>
</div>
);
});
SectionHeaderRow.displayName = 'SectionHeaderRow';
/**
* The main title of the SectionHeader.
*/
export const SectionHeaderTitle = React.forwardRef<
HTMLHeadingElement,
SectionHeaderTitleProps
>(({ children, className, ...props }, ref) => {
return (
<h2
ref={ref}
className={cn('heading-4-semi-bold min-w-0 truncate', className)}
{...props}
>
{children}
</h2>
);
});
SectionHeaderTitle.displayName = 'SectionHeaderTitle';
/**
* Optional count indicator, typically displayed next to the title.
* Example: "(30)"
*/
export const SectionHeaderCount = React.forwardRef<
HTMLSpanElement,
SectionHeaderCountProps
>(({ children, className, ...props }, ref) => {
return (
<span
ref={ref}
className={cn('text-muted heading-4-semi-bold shrink-0', className)}
{...props}
>
{children}
</span>
);
});
SectionHeaderCount.displayName = 'SectionHeaderCount';
/**
* Optional hint element, typically used for tooltip triggers.
* Example: Information icon
*/
export const SectionHeaderHint = React.forwardRef<
HTMLDivElement,
SectionHeaderHintProps
>(({ children, className, ...props }, ref) => {
return (
<div
ref={ref}
className={cn('flex shrink-0 items-center', className)}
{...props}
>
{children}
</div>
);
});
SectionHeaderHint.displayName = 'SectionHeaderHint';
/**
* Optional description text below the title row.
*/
export const SectionHeaderDescription = React.forwardRef<
HTMLParagraphElement,
SectionHeaderDescriptionProps
>(({ children, className, ...props }, ref) => {
return (
<p ref={ref} className={cn('text-muted body-3 mt-4', className)} {...props}>
{children}
</p>
);
});
SectionHeaderDescription.displayName = 'SectionHeaderDescription';
/**
* Container for action elements (buttons, links, etc.) on the right side.
*/
export const SectionHeaderAction = React.forwardRef<
HTMLDivElement,
SectionHeaderActionProps
>(({ children, className, ...props }, ref) => {
return (
<div
ref={ref}
className={cn('ml-auto flex shrink-0 items-center', className)}
{...props}
>
{children}
</div>
);
});
SectionHeaderAction.displayName = 'SectionHeaderAction';
/**
* A flexible SectionHeader component that provides a composable structure for section headers
* with title, count, hint, description, and action elements.
*
* @see {@link https://ldls.vercel.app/?path=/docs/communication-sectionheader--docs Storybook}
*
* @warning The `className` prop should only be used for layout adjustments like margins or positioning.
* Do not use it to modify the SectionHeader's core appearance (colors, padding, etc).
*
* @example
* import {
* SectionHeader,
* SectionHeaderRow,
* SectionHeaderTitle,
* SectionHeaderCount,
* SectionHeaderHint,
* SectionHeaderDescription,
* SectionHeaderAction
* } from '@ledgerhq/lumen-ui-react';
*
* // Basic SectionHeader with title only
* <SectionHeader>
* <SectionHeaderRow>
* <SectionHeaderTitle>Section Title</SectionHeaderTitle>
* </SectionHeaderRow>
* </SectionHeader>
*
* // Complete SectionHeader with all features
* <SectionHeader>
* <SectionHeaderRow>
* <SectionHeaderTitle>Section Title</SectionHeaderTitle>
* <SectionHeaderCount>(30)</SectionHeaderCount>
* <SectionHeaderHint>
* <Tooltip>
* <TooltipTrigger asChild>
* <Information size={12} className="text-muted" />
* </TooltipTrigger>
* <TooltipContent>Additional information</TooltipContent>
* </Tooltip>
* </SectionHeaderHint>
* <SectionHeaderAction>
* <Link href="/action" appearance="accent" size="sm">View All</Link>
* </SectionHeaderAction>
* </SectionHeaderRow>
* <SectionHeaderDescription>
* This is a description of the section
* </SectionHeaderDescription>
* </SectionHeader>
*/
export const SectionHeader = React.forwardRef<
HTMLDivElement,
SectionHeaderProps
>(({ children, className, ...props }, ref) => {
return (
<div ref={ref} className={cn('flex flex-col', className)} {...props}>
{children}
</div>
);
});
SectionHeader.displayName = 'SectionHeader';
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export const Subheader = ({ | ||
| lx, | ||
| style, | ||
| children, | ||
| ...props | ||
| }: SubheaderProps) => { | ||
| const styles = useStyleSheet( | ||
| (t) => ({ | ||
| container: { | ||
| width: '100%', | ||
| flexDirection: 'column', | ||
| gap: t.spacings.s4, | ||
| }, | ||
| }), | ||
| [], | ||
| ); | ||
|
|
||
| return ( | ||
| <View style={[styles.container, style]} {...props}> | ||
| {children} | ||
| </View> | ||
| ); |
Copilot
AI
Jan 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Subheader component accepts an lx prop in its types but never uses it. The prop is destructured from props but not applied to the View component. Either the prop should be processed using a styling utility (like other components use createStyledView or useResolveViewStyle), or it should be removed from the type definition if not needed.
| className={cn('flex items-center justify-between gap-8', className)} | ||
| {...props} | ||
| > | ||
| <div className='flex min-w-0 flex-1 items-center gap-6'>{children}</div> |
Copilot
AI
Jan 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SectionHeaderRow has an extra wrapper div that creates a nested structure. The children are wrapped in a div with flex-1, but this may cause unexpected layout behavior. The SectionHeaderAction component uses ml-auto to position itself at the right, but with this wrapper structure, all children are constrained within the inner flex container, making the justify-between on the outer container ineffective. Consider simplifying the layout or ensuring the structure supports the intended design.
| className={cn('flex items-center justify-between gap-8', className)} | |
| {...props} | |
| > | |
| <div className='flex min-w-0 flex-1 items-center gap-6'>{children}</div> | |
| className={cn('flex min-w-0 items-center justify-between gap-8', className)} | |
| {...props} | |
| > | |
| {children} |
| <Link href='https://example.com' appearance='accent' size='sm'> | ||
| Action | ||
| </Link> | ||
| </SubheaderAction> |
Copilot
AI
Jan 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation imports and uses a Link component inside SubheaderAction in React Native, but Link is a web component that doesn't exist in the React Native UI library. This example will not work. The SubheaderAction already provides interactive text styling, so children should be plain text or React Native components, not web Links.
| <Link href='https://example.com' appearance='accent' size='sm'> | |
| Action | |
| </Link> | |
| </SubheaderAction> | |
| Action | |
| </SubheaderAction> |
| <SubheaderAction onPress={handleAction}> | ||
| <Link href='#'>Link</Link> | ||
| </SubheaderAction> |
Copilot
AI
Jan 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Do's and Don'ts examples show using a Link component inside SubheaderAction in React Native, but Link is a web-only component that doesn't exist in the React Native library. These examples should use plain text children instead, consistent with the actual implementation and the sandbox example.
| <SubheaderAction onPress={handleAction}> | ||
| <Link href='#'>Link</Link> | ||
| </SubheaderAction> |
Copilot
AI
Jan 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Don't example also uses a Link component inside SubheaderAction in React Native, but Link is a web-only component. This should be updated to use plain text to match the actual React Native implementation.
| @@ -0,0 +1,191 @@ | |||
| import { cn } from '@ledgerhq/lumen-utils-shared'; | |||
Copilot
AI
Jan 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new SectionHeader component is not exported from the main Components index file. While the component is fully implemented with types, stories, and sub-components, it's not accessible to consumers of the library because there's no export statement in libs/ui-react/src/lib/Components/index.ts. Add the export to make this component available.
| <SubheaderAction> | ||
| <Link href='#' appearance='accent' size='sm'> | ||
| Action | ||
| </Link> | ||
| </SubheaderAction> |
Copilot
AI
Jan 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SubheaderAction component is missing the required onPress prop. According to the type definition in types.ts, onPress is a required prop for SubheaderActionProps, but it's not being provided in the Figma integration example.
| <button | ||
| onClick={onPress} | ||
| className={cn('text-interactive body-2 ml-auto shrink-0 pl-8', className)} | ||
| {...props} | ||
| > | ||
| {children} | ||
| </button> |
Copilot
AI
Jan 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SubheaderAction component is rendering a button element but doesn't specify type='button'. Without this attribute, buttons inside forms will default to type='submit', which could cause unintended form submissions. Add type='button' to the button element.
| if (onPress) { | ||
| return ( | ||
| <Pressable lx={lx} style={styles.container} onPress={onPress} {...props}> | ||
| {children} | ||
| </Pressable> | ||
| ); | ||
| } | ||
|
|
||
| return ( | ||
| <View style={[styles.container, style]} {...props}> | ||
| {children} | ||
| </View> | ||
| ); |
Copilot
AI
Jan 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent handling of style props between interactive and non-interactive rows. When onPress is provided, the lx prop is passed to Pressable but not the style prop. When onPress is not provided, the style prop is applied but lx is ignored. This creates inconsistent behavior where developers cannot reliably use both styling mechanisms across both states.
| <SubheaderAction onPress={() => console.log('Action clicked')}> | ||
| <Link href='https://ledger.com' appearance='accent' size='sm'> | ||
| View More | ||
| </Link> | ||
| </SubheaderAction> |
Copilot
AI
Jan 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation shows using a Link component inside SubheaderAction in React Native, but Link is a web component and doesn't exist in the React Native UI library. This example will not work in React Native. The SubheaderAction in React Native already renders text with interactive styling, so the children should be plain text, not a Link component.
| export const SubheaderRow = ({ | ||
| children, | ||
| onPress, | ||
| className, | ||
| ...props | ||
| }: SubheaderRowProps) => { | ||
| const Component = onPress ? 'button' : 'div'; | ||
|
|
||
| return ( | ||
| <Component | ||
| className={cn( | ||
| 'flex items-center gap-4', | ||
| onPress && 'cursor-pointer', | ||
| className, | ||
| )} | ||
| onClick={onPress} | ||
| {...props} | ||
| > | ||
| {children} | ||
| </Component> | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[low]: same here, onClick instead of onPress
[low]: We might add a hover state on the row that change the opacity, wdyt ?
We didn't had specifications from Laurine on that, but we can ask
| export const SubheaderHint = ({ content }: SubheaderHintProps) => { | ||
| return <div className='flex shrink-0 items-center'>{content}</div>; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[mid]: We didn't finished the discussion with Laurine on the SubHeaderHint, on the specs we considered it as optional.
But if we decide to add it as a sub-component, we want to add by default the whole information InteractiveIcon here :
<div className='flex shrink-0 items-center'>{content}
<InteractiveIcon iconType='stroked'>
<Information />
</InteractiveIcon>
</div>So consumers can use the tooltip like that:
<Tooltip>
<TooltipTrigger>
<SubHeaderHint>
</TooltipTrigger>
// ...
</Tooltip>
gamegee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const meta: Meta<typeof Subheader> = { | ||
| component: Subheader, | ||
| title: 'Communication/Subheader', | ||
| decorators: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| SubheaderRow, | ||
| SubheaderTitle, | ||
| SubheaderCount, | ||
| SubheaderHint, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have we not decided to call this SubheaderInfo instead ?
aammami-ledger
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.



Closes DLS-479.