feat(ui): update Bible version picker to match Figma#267
feat(ui): update Bible version picker to match Figma#267jaredhightower-youversion wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: 3272c83 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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 |
jaredhightower-youversion
requested review from
bmanquen and
cameronapak
and removed request for
cameronapak
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -680,11 +699,12 @@ function Content({ open, onRequestClose }: BibleVersionPickerContentProps = {}) | |||
| > | |||
| <ItemMedia | |||
| variant="icon" | |||
| className="yv:rounded-[8px] yv:size-12 yv:border-border yv:flex yv:flex-col yv:justify-center yv:items-center" | |||
| className="yv:rounded-md yv:size-16 yv:bg-secondary yv:border-border yv:flex yv:flex-col yv:justify-center yv:items-center" | |||
| > | |||
| <VersionAbbreviationIcon text={version.localized_abbreviation} /> | |||
| </ItemMedia> | |||
| <ItemContent> | |||
| <VersionPublisherName organizationId={version.organization_id} /> | |||
| <ItemTitle className="yv:line-clamp-2 yv:text-left">{version.title}</ItemTitle> | |||
There was a problem hiding this comment.
VersionPublisherName is mounted as a separate component for every row in both the "All Versions" and "Recent Versions" lists. Each instance independently calls useOrganization, which calls useOrganizationsClient (creating its own memoized ApiClient/OrganizationsClient per instance) and fires its own GET /v1/organizations/:id request. For a language with 50 Bible versions that all share a handful of publishers, this produces up to 50 concurrent requests with no deduplication at the React layer.
The standard fix is to hoist the fetching: collect the unique organization_id values from filteredVersions, fetch them at the Content level (or pass data down from the context), and pass the resolved name as a prop to each row. Alternatively, a shared organization cache via React context would deduplicate across all rows.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/src/components/bible-version-picker.tsx
Line: 666-708
Comment:
**N+1 API requests per list render**
`VersionPublisherName` is mounted as a separate component for every row in both the "All Versions" and "Recent Versions" lists. Each instance independently calls `useOrganization`, which calls `useOrganizationsClient` (creating its own memoized `ApiClient`/`OrganizationsClient` per instance) and fires its own `GET /v1/organizations/:id` request. For a language with 50 Bible versions that all share a handful of publishers, this produces up to 50 concurrent requests with no deduplication at the React layer.
The standard fix is to hoist the fetching: collect the unique `organization_id` values from `filteredVersions`, fetch them at the `Content` level (or pass data down from the context), and pass the resolved `name` as a prop to each row. Alternatively, a shared organization cache via React context would deduplicate across all rows.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| import { YouVersionContext } from './context'; | ||
| import { ApiClient, OrganizationsClient } from '@youversion/platform-core'; | ||
|
|
||
| export function useOrganizationsClient(): OrganizationsClient { |
There was a problem hiding this comment.
The file is named useOrganizationClient.ts (singular) but exports useOrganizationsClient (plural). Every other client hook in this package follows the convention of matching the file name to the primary export (e.g., useBibleClient.ts → useBibleClient). Renaming the file to useOrganizationsClient.ts would align it with the rest of the package and make it easier to locate by export name.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/hooks/src/useOrganizationClient.ts
Line: 7
Comment:
**File name doesn't match exported symbol**
The file is named `useOrganizationClient.ts` (singular) but exports `useOrganizationsClient` (plural). Every other client hook in this package follows the convention of matching the file name to the primary export (e.g., `useBibleClient.ts` → `useBibleClient`). Renaming the file to `useOrganizationsClient.ts` would align it with the rest of the package and make it easier to locate by export name.
How can I resolve this? If you propose a fix, please make it concise.
cameronapak
left a comment
cameronapak
left a comment
There was a problem hiding this comment.
Please address Greptile's comments and then let me know. The PR looks good to me! If you can create a video overviewing the UI changes, that would be very helpful. Please ping me on Teams or here!
|
@cameronapak @bmanquen one thing I noticed when I was when I was making the PR updates, the Figma design has the following fonts that it's using "Untitled Serif" and "Aktiv Grotesk App". Is this something that we want to only apply to this component because I feel like this is more of a wider change that could affect all of the other UI components as well? How would you prefer I go about making this change? |
4 participants
Summary
BibleVersionPicker: abbreviation tile resized (48→64px, bg-secondary), tighter typography (px-1.5,leading-[1.03],text-foreground)VersionPublisherNamesub-component fetches + renders organization name under each versionOrganizationsClientinpackages/core(Zod-validated UUID, GET/v1/organizations/:id)useOrganizationsClient+useOrganizationhooks inpackages/hooksorganization_idthreaded intoRecentVersiontype so recents also display publisherPR Description
What changed
BibleVersionPickertile: sizesize-12→size-16, backgroundbg-secondary, border radiusrounded-md; abbreviation text usespx-1.5 / leading-[1.03] / text-foreground— matches Figma specVersionPublisherNamecomponent renders organization name aen loading orunavailable — no layout shift)OrganizationsCgetOrganization(organizationId)` with UUID validation via ZoduseOrganization/useOrganizationsClient(packages/hooks): standarduseApiDatawrapper, memoized client scoped toYouVersionContextpackages/uipatch)Why
Align
BibleVersionigntile size, background, and publisher attribution were all missing or wrong.Figma Link:
https://www.figma.com/design/temXvWs3FR8DZdM8C17ajf/YVP---Reader-SDK-Design-v2?node-id=1-6698&m=dev
Screenshots, Images, Videos, etc
Screen.Recording.2026-06-23.at.2.50.58.PM.mov
Greptile Summary
This PR updates the
BibleVersionPickertile to match the Figma spec (48→64px,bg-secondary,rounded-md) and adds publisher attribution via a newVersionPublisherNamesub-component backed by a newOrganizationsClientinpackages/coreanduseOrganization/useOrganizationsClienthooks inpackages/hooks.packages/core:OrganizationsClientwith UUID-validatedgetOrganization(), Zod response parsing, and MSW test coverage.packages/hooks:useOrganizationwrapsuseApiDatawith an empty-ID guard;useOrganizationsClientmemoizes the client fromYouVersionContext. File name (useOrganizationClient.ts) diverges from the plural export name.packages/ui: Tile restyled,organization_idthreaded intoRecentVersion, andVersionPublisherNamemounted per-row — each instance independently callsuseOrganizationand fires its own API request, which can produce N concurrent fetches when rendering a full version list.Confidence Score: 3/5
The core and hooks packages are safe to merge as-is, but the UI component needs the per-row fetching strategy reconsidered before shipping.
Each rendered version row mounts its own VersionPublisherName instance, which independently calls useOrganization and fires a separate HTTP request. A language with 50 Bible versions can produce 50+ concurrent GET /v1/organizations/:id calls with no deduplication at the React layer. The new client, hook, and test coverage are otherwise solid.
packages/ui/src/components/bible-version-picker.tsx — the VersionPublisherName mounting strategy inside the version list loop
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant UI as BibleVersionPicker (Content) participant VPN as VersionPublisherName (×N rows) participant Hook as useOrganization participant Client as OrganizationsClient participant API as GET /v1/organizations/:id UI->>VPN: render row (version.organization_id) Note over VPN,API: Repeated for every row with an organization_id VPN->>Hook: useOrganization(organizationId) Hook->>Client: getOrganization(organizationId) Client->>API: "HTTP GET /v1/organizations/{uuid}" API-->>Client: Organization JSON Client-->>Hook: Organization (Zod-parsed) Hook-->>VPN: "{ organization, loading, error }" VPN-->>UI: renders org name (or null while loading)%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant UI as BibleVersionPicker (Content) participant VPN as VersionPublisherName (×N rows) participant Hook as useOrganization participant Client as OrganizationsClient participant API as GET /v1/organizations/:id UI->>VPN: render row (version.organization_id) Note over VPN,API: Repeated for every row with an organization_id VPN->>Hook: useOrganization(organizationId) Hook->>Client: getOrganization(organizationId) Client->>API: "HTTP GET /v1/organizations/{uuid}" API-->>Client: Organization JSON Client-->>Hook: Organization (Zod-parsed) Hook-->>VPN: "{ organization, loading, error }" VPN-->>UI: renders org name (or null while loading)Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "feat(core,hooks): add Organizations clie..." | Re-trigger Greptile