-
Notifications
You must be signed in to change notification settings - Fork 5
fix(ui): collapse the BibleCard error state into one alert region #321
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,7 @@ | ||
| --- | ||
| '@youversion/platform-react-ui': patch | ||
| --- | ||
|
|
||
| Fix the `BibleCard` error state announcing two alerts, and keep the version picker usable while an error is showing. The "Error" label stays in the header slot but drops its `role="alert"` and `aria-live`, leaving the message block in the card body as the only alert region. The picker no longer disappears on error, so a 404 has an in-card fix: switch to a version that carries the passage. | ||
|
|
||
| The shared message block also drops a redundant `aria-live` and hides its icon with `aria-hidden`, so `VerseOfTheDay` and standalone `BibleTextView` pick up the same accessibility fixes. Their visible text is unchanged, and neither gains an "Error" label. The eight status-aware messages, their six locales, and how errors are derived are untouched. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -186,6 +186,7 @@ export const Error: Story = { | |
| args: { | ||
| reference: 'LUK.1.39-45', | ||
| versionId: 111, | ||
| showVersionPicker: true, | ||
|
Collaborator
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 makes an
Adding handlers for the languages/versions endpoints fixes this and has a second benefit: it makes the manual verification step in the PR description ("open the version picker and select a different version… make sure the card recovers") actually reproducible from this story, which it currently isn't. |
||
| }, | ||
| tags: ['integration'], | ||
| parameters: { | ||
|
|
@@ -206,12 +207,26 @@ export const Error: Story = { | |
| play: async ({ canvasElement }) => { | ||
| const canvas = within(canvasElement); | ||
|
|
||
| // The header slot carries the "Error" label; the body block is the one alert. | ||
| await waitFor(async () => { | ||
| await expect(canvas.getByRole('heading', { level: 2, name: /error/i })).toBeInTheDocument(); | ||
| const errorMessages = canvas.getAllByText( | ||
| 'The Bible service is having trouble right now. Please try again in a moment.', | ||
| ); | ||
| await expect(errorMessages.length).toBeGreaterThan(0); | ||
| }); | ||
|
|
||
| const alerts = canvas.getAllByRole('alert'); | ||
|
|
||
| await expect(alerts).toHaveLength(1); | ||
| await expect(alerts[0]).toHaveTextContent( | ||
| 'The Bible service is having trouble right now. Please try again in a moment.', | ||
| ); | ||
|
|
||
| // The picker is the in-card recovery path: a 404 is fixed by switching versions. | ||
| const versionPickerButton = await canvas.findByRole('button', { | ||
| name: /change bible version/i, | ||
| }); | ||
|
|
||
| await waitFor(async () => { | ||
| await expect(versionPickerButton).toBeEnabled(); | ||
| await expect(versionPickerButton).toHaveTextContent(/NIV/i); | ||
| }); | ||
|
Collaborator
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. Should we assert that changing the version clears the error in this test? |
||
| }, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,8 +6,17 @@ import { render, act, within, waitFor } from '@testing-library/react'; | |
| import userEvent from '@testing-library/user-event'; | ||
| import { BibleCard } from './bible-card'; | ||
| import type { FootnoteData } from './verse'; | ||
| import { usePassage, useVersion, useTheme } from '@youversion/platform-react-hooks'; | ||
| import type { BiblePassage, BibleVersion } from '@youversion/platform-core'; | ||
| import { | ||
| useFilteredVersions, | ||
| useLanguage, | ||
| useLanguages, | ||
| useOrganizations, | ||
| usePassage, | ||
| useTheme, | ||
| useVersion, | ||
| useVersions, | ||
| } from '@youversion/platform-react-hooks'; | ||
| import type { BiblePassage, BibleVersion, Language } from '@youversion/platform-core'; | ||
|
|
||
| vi.mock('@youversion/platform-react-hooks'); | ||
|
|
||
|
|
@@ -156,6 +165,90 @@ describe('BibleCard - Delayed spinner', () => { | |
| }); | ||
| }); | ||
|
|
||
| describe('BibleCard - Error state', () => { | ||
| function createError(message: string, status?: number): Error { | ||
|
Collaborator
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. Two smaller things on the new test block:
These five jsdom tests largely restate the updated |
||
| return Object.assign(new Error(message), status === undefined ? {} : { status }); | ||
| } | ||
|
|
||
| beforeEach(() => { | ||
| vi.mocked(useTheme).mockReturnValue('light'); | ||
| vi.mocked(useVersion).mockReturnValue({ | ||
| version: mockVersion, | ||
| loading: false, | ||
| error: null, | ||
| refetch: vi.fn(), | ||
| }); | ||
| vi.mocked(usePassage).mockReturnValue({ | ||
| passage: null, | ||
| loading: false, | ||
| error: createError('Request failed with status 503', 503), | ||
| refetch: vi.fn(), | ||
| }); | ||
| }); | ||
|
|
||
| it('should render exactly one alert region', () => { | ||
| const { container } = render(<BibleCard reference="JHN.3.16" versionId={3034} />); | ||
|
|
||
| expect(within(container).getAllByRole('alert')).toHaveLength(1); | ||
| }); | ||
|
|
||
| it('should show the status message in that one alert region', () => { | ||
| const { container } = render(<BibleCard reference="JHN.3.16" versionId={3034} />); | ||
| const alert = within(container).getByRole('alert'); | ||
|
|
||
| expect(alert).toHaveTextContent( | ||
| 'The Bible service is having trouble right now. Please try again in a moment.', | ||
| ); | ||
| }); | ||
|
|
||
| it('should render the error heading in the header slot', () => { | ||
| const { container } = render(<BibleCard reference="JHN.3.16" versionId={3034} />); | ||
|
|
||
| expect(within(container).getByRole('heading', { level: 2 })).toHaveTextContent('Error'); | ||
| }); | ||
|
|
||
| it('should not render a loading spinner while an error is set', () => { | ||
| const { container } = render(<BibleCard reference="JHN.3.16" versionId={3034} />); | ||
|
|
||
| expect(within(container).queryByRole('status')).toBeNull(); | ||
| }); | ||
|
|
||
| it('should keep the version picker usable so a bad version can be swapped', () => { | ||
| // The version picker mounts its own hook tree. The file-level auto-mock | ||
| // returns undefined for each one, so give them values here. | ||
| vi.mocked(useLanguages).mockReturnValue({ | ||
| languages: { data: [] as Language[], next_page_token: null }, | ||
| loading: false, | ||
| error: null, | ||
| refetch: vi.fn(), | ||
| }); | ||
| vi.mocked(useLanguage).mockReturnValue({ | ||
| language: { id: 'en', language: 'English', display_names: { en: 'English' } } as Language, | ||
| loading: false, | ||
| error: null, | ||
| refetch: vi.fn(), | ||
| }); | ||
| vi.mocked(useVersions).mockReturnValue({ | ||
| versions: { data: [], next_page_token: null }, | ||
| loading: false, | ||
| error: null, | ||
| refetch: vi.fn(), | ||
| }); | ||
| vi.mocked(useFilteredVersions).mockReturnValue([]); | ||
| vi.mocked(useOrganizations).mockReturnValue({ organizations: new Map() }); | ||
|
|
||
| const { container } = render( | ||
| <BibleCard reference="JHN.3.16" versionId={3034} showVersionPicker />, | ||
| ); | ||
|
|
||
| const picker = within(container).getByRole('button', { name: /change bible version/i }); | ||
|
|
||
| expect(picker).toBeInTheDocument(); | ||
| expect(picker).toBeEnabled(); | ||
| expect(picker).toHaveTextContent('BSB'); | ||
| }); | ||
|
Collaborator
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. Should we assert that a successful version change clears the error in this test or create another separate test to assert this? |
||
| }); | ||
|
|
||
| describe('BibleCard - onFootnotePress callback', () => { | ||
| const mockPassageWithFootnote: BiblePassage = { | ||
| id: 'JHN.1', | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -251,17 +251,20 @@ const VerseFootnoteButton = memo(function VerseFootnoteButton({ | |
| }); | ||
|
|
||
| /** | ||
| * Displays a verse-unavailable error message with a circular exclamation | ||
| * icon and descriptive text. | ||
| * Displays a verse-unavailable error message as one alert region: a circular | ||
| * exclamation icon and the status-aware message. | ||
| * | ||
| * The "Error" label lives in the BibleCard header slot, not here, so this block | ||
| * stays a single sentence. `role="alert"` already implies an assertive live | ||
| * region, so no `aria-live` is set, and the icon is hidden from screen readers. | ||
| */ | ||
| function VerseUnavailableMessage({ message }: { message: string }): React.ReactElement { | ||
| return ( | ||
| <div | ||
| role="alert" | ||
|
Collaborator
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. Removing
The doc comment added in this same hunk actually states this correctly (" This lands on If assertive is the intent, that's defensible for a failed load, but it should be stated as the deliberate change it is. If it isn't intended, keep |
||
| aria-live="polite" | ||
| className="yv:flex yv:items-center yv:justify-center yv:gap-2.5 yv:px-3 yv:py-2.5 yv:text-foreground" | ||
| > | ||
| <ExclamationCircle className="yv:size-5 yv:shrink-0 yv:text-foreground" /> | ||
| <ExclamationCircle className="yv:size-5 yv:shrink-0 yv:text-foreground" aria-hidden="true" /> | ||
| <p className="yv:m-0 yv:text-[13px] yv:font-medium yv:leading-tight">{message}</p> | ||
| </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.
"drops a redundant
aria-live" — same correction as theverse.tsxcomment: it wasn't redundant, it was an override, and removing it movesVerseOfTheDayand standaloneBibleTextViewfrom polite to assertive announcements.docs/review-guidelines.mdasks whether the code actually implements what the PR description claims, and whether consumer-visible changes are identified in the changeset. Since this ships to consumers of both those components, the changeset should say the announcement becomes assertive rather than describing it as a no-op cleanup.