Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends the React Native experimental DataList / DetailsItem list system by adding support for a new “training” item type, and demonstrates it in the playground showcase.
Changes:
- Add
trainingas a newContentvariant inDetailsItemand render it viaDataList.TrainingItem. - Implement
DataList.TrainingItemto display a training thumbnail, title, metadata tags, and progress UI. - Update the RN playground
DetailsItemsListShowcaseto include a training example entry.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| packages/react-native/src/components/experimental/Lists/DetailsItem/index.tsx | Adds training to the Content union and renders DataList.TrainingItem when selected. |
| packages/react-native/src/components/experimental/Lists/DataList/index.tsx | Introduces the new TrainingItem implementation and exports it via DataList. |
| packages/react-native/playground/components/DetailsItemsListShowcase/index.tsx | Adds a “Training” item to the showcase data to demonstrate the new list item. |
| const TrainingItem = ({ | ||
| action, | ||
| name, | ||
| thumbnailUrl, | ||
| metadata, | ||
| }: TrainingItemProps) => { | ||
| const { hours, dueDate, status, progress } = metadata; | ||
| const renderDueDate = () => { |
There was a problem hiding this comment.
metadata is optional in TrainingItemProps, but the component destructures it directly (const { ... } = metadata). This will throw at runtime when metadata is undefined and should also fail TypeScript strictness. Consider defaulting to metadata ?? {} (and handling dueDate/status/progress accordingly) or making metadata required if the UI always needs it.
| const statusColor: Record<typeof status, NewColor> = { | ||
| enrolled: "smoke", | ||
| completed: "viridian", | ||
| }; | ||
| return ( | ||
| <View className="flex flex-row items-start gap-2 pt-1"> | ||
| {thumbnailUrl ? ( | ||
| <View className="aspect-square h-20 w-20 rounded-sm"> | ||
| <Image | ||
| style={{ width: "100%", height: "100%" }} | ||
| source={{ | ||
| uri: thumbnailUrl, | ||
| }} | ||
| aria-label={name} | ||
| /> | ||
| </View> | ||
| ) : ( | ||
| <View className="bg-f1-background-secondary aspect-square h-32 w-32 rounded-sm" /> | ||
| )} | ||
| <View className="flex flex-1 flex-col gap-2"> | ||
| <Text className="text-f1-background-inverse-primary line-clamp-2 text-lg font-medium"> | ||
| {name} | ||
| </Text> | ||
| <View className="flex flex-row flex-wrap gap-1"> | ||
| {hours && <RawTag icon={Clock} text={`${hours} hours`} noBorder />} | ||
| {dueDate && renderDueDate()} | ||
| </View> | ||
| {status !== "inprogress" && ( | ||
| <DotTag text={status} color={statusColor[status]} /> | ||
| )} |
There was a problem hiding this comment.
status can be undefined (and also includes "inprogress"), but the code renders <DotTag text={status} color={statusColor[status]} /> whenever status !== "inprogress". This can pass undefined to DotTag and index statusColor with an undefined key. Also Record<typeof status, NewColor> is not a valid mapping type here (it includes undefined/"inprogress" but the object doesn’t). Render the tag only when status is one of the mapped values, and type the map accordingly.
| type TrainingItemProps = { | ||
| name: string; | ||
| thumbnailUrl?: string; | ||
| metadata?: { | ||
| hours?: number; | ||
| dueDate?: { | ||
| date: string; | ||
| status?: "critical" | "warning"; | ||
| }; | ||
| status?: "enrolled" | "inprogress" | "completed"; | ||
| progress?: number; | ||
| }; | ||
| action?: ActionType; | ||
| }; | ||
|
|
||
| const TrainingItem = ({ | ||
| action, | ||
| name, | ||
| thumbnailUrl, | ||
| metadata, | ||
| }: TrainingItemProps) => { |
There was a problem hiding this comment.
action is accepted and destructured in TrainingItem, but never used. This will typically trigger no-unused-vars and it also means callers can’t make the training row actionable like the other DataList items. Either wire action into a Pressable/ItemContainer pattern or remove the prop from TrainingItemProps to avoid a misleading API.
| import React, { ReactElement } from "react"; | ||
| import { View, Text, Image } from "react-native"; | ||
| import { AlertTag, RawTag } from "src/components/exports"; | ||
| import { Calendar, Clock } from "src/icons/app"; | ||
|
|
||
| import { cn } from "../../../../lib/utils" | ||
| import { CompanyAvatar } from "../../../Avatars/CompanyAvatar" | ||
| import { PersonAvatar } from "../../../Avatars/PersonAvatar" | ||
| import { TeamAvatar } from "../../../Avatars/TeamAvatar" | ||
| import { IconType } from "../../../Icon" | ||
| import { DotTag, DotTagProps } from "../../../Tags/DotTag" | ||
|
|
||
| import { ItemContainer } from "./ItemContainer" | ||
| import { cn } from "../../../../lib/utils"; | ||
| import { CompanyAvatar } from "../../../Avatars/CompanyAvatar"; | ||
| import { PersonAvatar } from "../../../Avatars/PersonAvatar"; | ||
| import { TeamAvatar } from "../../../Avatars/TeamAvatar"; | ||
| import { IconType } from "../../../Icon"; | ||
| import { DotTag, DotTagProps, NewColor } from "../../../Tags/DotTag"; | ||
| import { ItemContainer } from "./ItemContainer"; |
There was a problem hiding this comment.
The repository formatter config sets semi: false (see .oxfmtrc.json), but this file now uses semicolons throughout. This is likely to cause format/lint churn in CI. Please run the configured formatter (oxfmt) on this file so it matches the repo style.
| export type DataListProps = { | ||
| children: ReactElement<Items>[] | ReactElement<Items> | ||
| label?: string | ||
| isHorizontalItem?: boolean | ||
| tableView?: boolean | ||
| } | ||
| children: ReactElement<Items>[] | ReactElement<Items>; | ||
| label?: string; | ||
| isHorizontalItem?: boolean; | ||
| tableView?: boolean; | ||
| }; | ||
|
|
||
| type Items = | ||
| | typeof Item | ||
| | typeof PersonItem | ||
| | typeof CompanyItem | ||
| | typeof TeamItem | ||
| | typeof TeamItem; |
There was a problem hiding this comment.
DataListProps.children is typed as ReactElement<Items>, but Items is a union of component types (typeof Item | ...), and ReactElement’s first type parameter represents props, not the component type. This makes the children typing inaccurate and can reject valid children (or fail to model what’s actually allowed). Consider switching to ReactNode/ReactElement without generics, or explicitly typing the element type via the second generic parameter and expanding the union to include the exported items (e.g. DotTagItem, TrainingItem).
| {name} | ||
| </Text> | ||
| <View className="flex flex-row flex-wrap gap-1"> | ||
| {hours && <RawTag icon={Clock} text={`${hours} hours`} noBorder />} |
There was a problem hiding this comment.
{hours && ...} will hide a valid value of 0 hours. If 0 is meaningful, prefer an explicit number check (e.g. hours !== undefined) so the tag renders correctly.
| {hours && <RawTag icon={Clock} text={`${hours} hours`} noBorder />} | |
| {hours !== undefined && ( | |
| <RawTag icon={Clock} text={`${hours} hours`} noBorder /> | |
| )} |
Description
Screenshots (if applicable)
[Link to Figma Design](Figma URL here)
Implementation details