YPE-1204: Fix verse error states + display reference#165
YPE-1204: Fix verse error states + display reference#165jaredhightower-youversion wants to merge 2 commits intomainfrom
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Greptile SummaryImproved user experience by ensuring Bible references are always visible and error states are clear and accessible. Core changes:
UI improvements:
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[BibleCard Component] --> B{passage?.reference available?}
B -->|Yes| C[Display server reference]
B -->|No| D{reference prop exists?}
D -->|Yes| E[formatUsfmForDisplay reference]
D -->|No| F[Display nothing]
E --> G[Show formatted USFM]
C --> H[Render header with reference]
G --> H
I[BibleTextView Component] --> J{API State?}
J -->|Loading| K[Show Loading...]
J -->|Error| L[VerseUnavailableMessage]
J -->|Success| M[Render Bible HTML]
L --> N[ExclamationCircle Icon + Error Text]
O[formatUsfmForDisplay] --> P[Split USFM by dots]
P --> Q{Has chapter & verse?}
Q -->|Yes| R[Format: BOOK C:V]
Q -->|No| S{Has chapter only?}
S -->|Yes| T[Format: BOOK C]
S -->|No| U[Format: BOOK]
Last reviewed commit: 980d393 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Looks beautiful. |
|
Still reviewing but I like what I see |
There was a problem hiding this comment.
task: @jaredhightower-youversion util is really cool, and at the same time I think we do not need it 😔.
It appears its existence was created to solve the issue of the BibleCard not showing the reference when loading the Bible reference and text. It makes sense to want to have something there so that when the data loads, then new text doesn't just appear out of thin air, causing the component to have some layout shifting.
I think it's better in this case to consider one of two options:
- Use a Skeleton component while the Bible reference text is loading
- Or, use a Loading spinner while the Bible reference text is loading
There was a problem hiding this comment.
question: If we use a skeleton component or a loading spinner and the network request fails, we wouldn't display a reference, right? Or is the intended behavior to keep the skeleton or spinner visible for reference when the verse is in a failed state?
The original intention for this util was to still display the reference even when the verse enters a failed state. This was also in the designs Tim provided. I guess another question is, do we already have another way to handle this within the SDK?
With Util
Without Util
{passage?.reference ? (
<h2 className="yv:font-bold yv:tracking-widest yv:text-xs yv:uppercase yv:text-foreground">
{passage.reference} {version?.localized_abbreviation}
</h2>
) : null}There was a problem hiding this comment.
Great question, man. Sorry for the delay.
The error state of the readable reference is a real concern. Tim's designs clearly show a Bible reference there, but...
I should've been more clear. My comment was specifically about the loading state, not the error state
I think we're actually dealing with two separate problems:
- Loading state: what to show while the API request is in-flight
- Error state: what to show when the request fails entirely
For loading, I'd still lean toward a skeleton or spinner rather than the util, since the formatted USFM util returns English-only text and won't scale as we support more languages in our copy (soon)
For the error state, I looked at how the Swift SDK handles this. They use structured reference data (BibleReference with separate bookUSFM, chapter, verseStart, verseEnd) and get localized book names from the API via BibleVersion.displayTitle(for:), so there is no hardcoded English mapping for Bible reference. But the more important/intriguing thing is, their error state also doesn't show a reference if the version data failed to load. So it's the same gap on both platforms
That communicates to me the right fix isn't a client-side English book name map, but rather a design decision that needs to be processed with Tim Watson
What do you think about scoping this PR to the loading spinner while the API is in-flight + the verse unavailbale copy & icon, and we can then split the error-state reference display into its own ticket? And, let's say we get an error on the verse loading in the BibleCard, then you can render an empty div to prevent layout shifting. That way your work isn't blocked
Thoughts, @jaredhightower-youversion?
There was a problem hiding this comment.
I don't mind removing the util, I just wanted to make sure that we're building to the design specs. Also, most of the work for the error state was done in this PR. Did you want me to remove that work and focus only on the loading-state rendering, or should we keep the changes for the error state in the PR (see ticket details here)?
With everything mentioned above, a clear next step action item would be as follows:
- Seek clarification from Tim Watson regarding the error state for references when a verse is in a failed state.
- Remove the reference uitl.
- Prevent layout shift for failed verse.
- Create a follow-up ticket for the loading state if we decide to keep error state changes in this PR.
@cameronapak, let me know what you think about this.
There was a problem hiding this comment.
Thank you for processing. I think the error state is the main thing. So everything else feels secondary. Would you agree?
2 participants
Summary
Problem
When using
passage?.referencedirectly inBibleCard, the reference header showed nothing while the API was loading or if the request failed. Users saw a blank space where the reference (e.g. "JOHN 3:16 NIV") should be.The error state in
BibleTextViewalso displayed a generic"We have run into an error..."message styled as Bible text, which was confusing and didn't match design specs.Solution
Core (
@youversion/platform-core)formatUsfmForDisplayutility that converts raw USFM strings (e.g."JHN.3.16") into human-readable references (e.g."JOHN 3:16")UsfmBookCodeandUsfmReferencetypes for type-safe USFM handling with IDE autocompleteUI (
@youversion/platform-react-ui)BibleCard: UsesdisplayReferencefallback pattern — shows server reference when available, falls back to client-formatted USFM during loading/error so the header is always visibleBibleTextView: Replaced generic error string with a dedicatedVerseUnavailableMessagecomponent featuring anExclamationCircleicon matching the Figma designExclamationCircleicon componentHow it works