-
Notifications
You must be signed in to change notification settings - Fork 38
feat(evo-react): add evo-details component #640
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@evo-web/marko": patch | ||
| --- | ||
|
|
||
| Add leading attribute tag to evo-details |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@evo-web/react": patch | ||
| --- | ||
|
|
||
| Add evo-details component | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,6 +55,60 @@ If a component is not listed below, it has **not been migrated yet** — keep us | |
|
|
||
| No prop changes. Global renames (Step 2) are sufficient. | ||
|
|
||
| ### `ebay-details` | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This list is going to get really long once everything's in here... maybe we should have a separate markdown file for each component?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah probably we should start moving around. |
||
|
|
||
| This component has a **new composite API** in evo-react. The flat-prop approach is replaced by named sub-components. | ||
|
|
||
| **Before:** | ||
|
|
||
| ```tsx | ||
| import { EbayDetails } from "@ebay/ebayui-core-react/ebay-details"; | ||
|
|
||
| <EbayDetails | ||
| text="Show me the details!" | ||
| size="small" | ||
| alignment="center" | ||
| leading={<Icon />} | ||
| onToggle={handler} | ||
| > | ||
| Content here | ||
| </EbayDetails>; | ||
| ``` | ||
|
|
||
| **After:** | ||
|
|
||
| ```tsx | ||
| import { | ||
| EvoDetails, | ||
| EvoDetailsSummary, | ||
| EvoDetailsLeading, | ||
| EvoDetailsLabel, | ||
| EvoDetailsContent, | ||
| } from "@evo-web/react"; | ||
|
|
||
| <EvoDetails size="small" alignment="center" onToggle={handler}> | ||
| <EvoDetailsSummary> | ||
| <EvoDetailsLeading> | ||
| <Icon /> | ||
| </EvoDetailsLeading> | ||
| <EvoDetailsLabel>Show me the details!</EvoDetailsLabel> | ||
| </EvoDetailsSummary> | ||
| <EvoDetailsContent>Content here</EvoDetailsContent> | ||
| </EvoDetails>; | ||
| ``` | ||
|
|
||
| **Prop changes:** | ||
|
|
||
| | ebayui-core-react | evo-react | Notes | | ||
| | ------------------------ | ----------------------------------- | ---------------------------------------------------------------------------- | | ||
| | `text: string` | `<EvoDetailsLabel>` sub-component | Move label text into `<EvoDetailsLabel>` inside `<EvoDetailsSummary>` | | ||
| | `leading?: ReactElement` | `<EvoDetailsLeading>` sub-component | Move leading element into `<EvoDetailsLeading>` inside `<EvoDetailsSummary>` | | ||
| | `as?: ElementType` | `as` prop on `<EvoDetailsContent>` | Move to the content sub-component | | ||
| | `onToggle` | `onToggle` | Same signature: `(event, { open: boolean }) => void` | | ||
| | `children` | `<EvoDetailsContent>` children | Wrap children in `<EvoDetailsContent>` | | ||
|
|
||
| **Important:** `<EvoDetailsLeading>` must appear before `<EvoDetailsLabel>` inside `<EvoDetailsSummary>` — order is not enforced by the component. | ||
|
|
||
| --- | ||
|
|
||
| ## Step 4 — Verify | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| # 5. Evo React Child Component Composition | ||
|
|
||
| **Date:** 2026-04-23 | ||
|
|
||
| ## Status | ||
|
|
||
| Accepted | ||
|
|
||
| ## Context | ||
|
|
||
| `@ebay/ui-core-react` uses `React.Children.map` / `findComponent` to introspect children and wire up composite component structures. This API is fragile, order-sensitive, and deprecated by React. | ||
|
|
||
| ### Alternatives considered | ||
|
|
||
| - **`summary={<span />}`** — requires `React.cloneElement` to inject BEM classes into the passed element, which is equally discouraged. | ||
| - **`summary={{ children: "", className, ...spanProps }}`** — avoids cloning but forces consumers to pass a plain object instead of JSX, which is poor DX. | ||
|
|
||
| ## Decision | ||
|
|
||
| `@evo-web/react` uses **named sub-components** (compound component pattern) instead of `React.Children` scanning. Shared state is passed via React context from the parent. Sub-components consume only what they need. | ||
|
|
||
| ```tsx | ||
| <EvoDetails size="small" onToggle={handler}> | ||
| <EvoDetailsSummary> | ||
| <EvoDetailsLeading> | ||
| <EvoIconLightbulb16 /> | ||
| </EvoDetailsLeading> | ||
| <EvoDetailsLabel>How do I get started?</EvoDetailsLabel> | ||
| </EvoDetailsSummary> | ||
| <EvoDetailsContent>Content</EvoDetailsContent> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we planning on always having <EvoDetails size="small" onToggle={handler}>
<EvoDetailsSummary>
<EvoDetailsLeading>
<EvoIconLightbulb16 />
</EvoDetailsLeading>
How do I get started?
</EvoDetailsSummary>
Content
</EvoDetails>
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not always, it will depend on the component and how skin organizes it. Having the component allow the developer to add other properties like class names to the container, but it will depend for each component might be different. |
||
| </EvoDetails> | ||
|
Comment on lines
+23
to
+31
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thoughts on using an object instead of individual components, like Radix does? Might make more sense for grouping, but I'm not sure <EvoDetails.Root size="small" onToggle={handler}>
<EvoDetails.Summary>
<EvoDetails.Leading>
<EvoIconLightbulb16 />
</EvoDetails.Leading>
<EvoDetails.Label>How do I get started?</EvoDetails.Label>
</EvoDetails.Summary>
<EvoDetails.Content>Content</EvoDetails.Content>
</EvoDetails.Root>
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can have both at some point like base-ui has. |
||
| ``` | ||
|
|
||
| All sub-components are exported from the package entry point. | ||
|
|
||
| ## Consequences | ||
|
|
||
| ### Positive | ||
|
|
||
| - More option for developers to customize the layout when possible. | ||
| - Better DX with simpler to use component APIs. | ||
| - Removes dependency of `React.Children` APIs and `React.cloneElement`. | ||
|
|
||
| ### Negative | ||
|
|
||
| - Child rendering order is the consumer's responsibility — the parent does not enforce it. | ||
| - The evo-react API will diverge from evo-marko's attribute tag pattern (`<@summary>`), which is a Marko 6-specific construct with no React equivalent. | ||
| - More verbose component APIs | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,5 @@ | ||||||||||||||
| <evo-details ...input> | ||||||||||||||
| <@summary>Details</@summary> | ||||||||||||||
| <@leading><evo-icon-lightbulb-16/></@leading> | ||||||||||||||
|
Comment on lines
+2
to
+3
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we nest
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will be wrong no? It will render the icon inside the summary text? But it is a sibling
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the markup it is a child of
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes but
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I guess you're right but it does mess with my mental model a little bit. I've also never been a fan of |
||||||||||||||
| Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum. | ||||||||||||||
| </evo-details> | ||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| # EvoDetails | ||
|
|
||
| ## Documentation | ||
|
|
||
| [Storybook](https://opensource.ebay.com/evo-web/react/main/?path=/docs/navigation-disclosure-evo-details--documentation) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| import { createContext, useContext } from "react"; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it was up to me I'd probably put all of the component parts in one file, you know better than I do though
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think having in a separate file prevents the risk of circular dependencies since we have multiple components. |
||
| import type { Size, Alignment } from "./types"; | ||
|
|
||
| export type DetailsContextValue = { | ||
| size?: Size; | ||
| alignment?: Alignment; | ||
| }; | ||
|
|
||
| export const DetailsContext = createContext<DetailsContextValue>({}); | ||
|
|
||
| export function useDetailsContext() { | ||
| return useContext(DetailsContext); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| import classNames from "classnames"; | ||
| import type { EvoDetailsContentProps } from "./types"; | ||
|
|
||
| export function EvoDetailsContent({ | ||
| as: Component = "div", | ||
| children, | ||
| className, | ||
| ...rest | ||
| }: EvoDetailsContentProps) { | ||
| return ( | ||
| <Component className={classNames("details__content", className)} {...rest}> | ||
| {children} | ||
| </Component> | ||
| ); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| import classNames from "classnames"; | ||
| import type { EvoDetailsLabelProps } from "./types"; | ||
|
|
||
| export function EvoDetailsLabel({ | ||
| children, | ||
| className, | ||
| ...rest | ||
| }: EvoDetailsLabelProps) { | ||
| return ( | ||
| <span className={classNames("details__label", className)} {...rest}> | ||
| {children} | ||
| </span> | ||
| ); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| import classNames from "classnames"; | ||
| import type { EvoDetailsLeadingProps } from "./types"; | ||
|
|
||
| export function EvoDetailsLeading({ | ||
| children, | ||
| className, | ||
| ...rest | ||
| }: EvoDetailsLeadingProps) { | ||
| return ( | ||
| <span className={classNames("details__leading", className)} {...rest}> | ||
| {children} | ||
| </span> | ||
| ); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| import classNames from "classnames"; | ||
| import type { EvoDetailsSummaryProps } from "./types"; | ||
| import { useDetailsContext } from "./context"; | ||
| import { EvoIconChevronDown16 } from "../evo-icon/icons/evo-icon-chevron-down-16"; | ||
|
|
||
| export function EvoDetailsSummary({ | ||
| children, | ||
| className, | ||
| ...rest | ||
| }: EvoDetailsSummaryProps) { | ||
| const { size, alignment } = useDetailsContext(); | ||
|
|
||
| return ( | ||
| <summary | ||
| className={classNames( | ||
| "details__summary", | ||
| size === "small" && "details__summary--small", | ||
| alignment === "center" && "details__summary--center", | ||
| className, | ||
| )} | ||
| {...rest} | ||
| > | ||
| {children} | ||
| <span className="details__icon" hidden> | ||
| <EvoIconChevronDown16 /> | ||
| </span> | ||
| </summary> | ||
| ); | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.