fix: artist bio rendering on Artwork screen - #13842
Conversation
Needed because for *unlisted* works, the artist bio card is rendered with inverted colors, and this needs to work correctly regardless of theme.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
biographyBlurb(format: HTML) returns markup, and slicing/measuring it by raw character count let tags eat the truncation budget and could cut mid-tag or mid-attribute, breaking the markup. truncateHtml now counts and cuts only on visible (non-tag) characters, applied to both AboutArtist and Biography which share this truncation pattern. Co-Authored-By: Claude Sonnet 5 <[email protected]>
The Read More/Less toggle used palette-mobile's LinkText, which defaults accessibilityRole to "link" — misleading for a toggle that doesn't navigate. ReadMore set this explicitly to "button"; restore that here. Co-Authored-By: Claude Sonnet 5 <[email protected]>
Two issues from PR review: the char-by-char scanner counted entity references (e.g. "&") one character at a time, so a cut could land mid-entity and a valid entity's raw length inflated the visible budget. Separately, once the budget was spent the scanner kept consuming any following tag — including a new opening tag like <h3> — leaving it dangling open when the caller appends "... " right after it. Fix: treat a full "&name;"/"{"/"" run as one visible character in both truncateHtml and visibleHtmlTextLength, and once the budget is spent, only continue consuming closing tags (to finish elements already open) rather than any tag. Co-Authored-By: Claude Sonnet 5 <[email protected]>
The color prop only reached text through tagsStyles entries for a/p/li/em/h1/h2/h3. Anything outside those — bare root text, or tags like strong/blockquote/ol with no explicit entry — kept react-native-render-html's default color, rendering dark-on-dark on the inverted (unlisted) card. This is reachable today: truncateHtml can stop right after a closing block tag, and AboutArtist appends "... " outside any element at that point. baseStyle applies to the whole render tree, closing the gap for every tag at once instead of enumerating more tagsStyles entries. Co-Authored-By: Claude Sonnet 5 <[email protected]>
They answered the same "what counts as visible" question two
different ways — one by regex-stripping tags, the other by walking
characters with an inTag flag — and could disagree on malformed
input (e.g. an unterminated "<" tag), since only one of the two
strips it. Both callers then risked "canExpand: true" alongside a
truncatedText that was already the full string, making Read More a
no-op.
truncateHtml now returns { text, wasTruncated } from a single pass,
so the two can no longer drift apart by construction. Simplifies
both call sites, which no longer need two separate function calls.
Co-Authored-By: Claude Sonnet 5 <[email protected]>
| * without ever cutting in the middle of a tag, an attribute, or an entity reference | ||
| * (e.g. `&`). Once the budget is spent, still emits any tags needed to close | ||
| * elements that are already open, but stops before a new tag would open. |
There was a problem hiding this comment.
The doc comment overstates what the scanner does. It never emits closing tags — it only passes through ones already present in the source at the cut point. In the common case the budget runs out mid-element and the output has a dangling open tag:
truncateHtml("<p>Hello world</p>", 5) → "<p>Hello" (your own test on line 30 of the spec asserts exactly this).
That's fine in practice since react-native-render-html's parser auto-closes, but the comment reads as a guarantee of balanced markup. Suggest wording closer to "passes through closing tags that follow the cut so already-open elements aren't left mid-list, but stops before a new tag would open."
| <RenderHtml | ||
| contentWidth={contentWidth} | ||
| source={{ html }} | ||
| baseStyle={{ color: color(textColor) }} |
There was a problem hiding this comment.
baseStyle carries only color, so text that doesn't match a tagsStyles entry gets the right color but not fontFamily: FONTS.regular. That path is reachable by design here: AboutArtist appends "... " after truncateHtml stops on a closing block tag, so the ellipsis is a root text node in the system font next to bio text in Unica77LL.
Adding fontFamily: FONTS.regular to baseStyle closes it. Don't spread variantStyles in as well — li deliberately omits lineHeight (line 92) and a baseStyle lineHeight would cascade back into it and undo that.
| const { text: truncatedText, wasTruncated: canExpand } = text | ||
| ? truncateHtml(text, textLimit) | ||
| : { text: undefined, wasTruncated: false } |
There was a problem hiding this comment.
The ternary only exists to avoid calling truncateHtml(null), and it types truncatedText as string | undefined for a value that's only read under a !!text guard. truncateHtml("", n) already returns { text: "", wasTruncated: false }:
| const { text: truncatedText, wasTruncated: canExpand } = text | |
| ? truncateHtml(text, textLimit) | |
| : { text: undefined, wasTruncated: false } | |
| const { text: truncatedText, wasTruncated: canExpand } = truncateHtml(text ?? "", textLimit) |
| it("renders HTML-formatted bio content without raw markdown markers", () => { | ||
| renderWithRelay({ | ||
| Artwork: () => ({ | ||
| isUnlisted: false, | ||
| displayArtistBio: true, | ||
| artists: [ | ||
| { | ||
| biographyBlurb: { | ||
| text: "<h3>Key Exhibitions</h3><ul><li>David Hockney, The Metropolitan Museum of Art, 2017</li></ul>", | ||
| }, | ||
| }, | ||
| ], | ||
| }), | ||
| }) | ||
|
|
||
| expect(screen.getByText("Key Exhibitions")).toBeOnTheScreen() | ||
| expect( | ||
| screen.getByText("David Hockney, The Metropolitan Museum of Art, 2017") | ||
| ).toBeOnTheScreen() | ||
| expect(screen.queryByText(/<h3>/)).not.toBeOnTheScreen() | ||
| }) |
There was a problem hiding this comment.
The unlisted/inverted variant is the reason the color prop was added to HTML, and nothing here asserts it. This test sets isUnlisted: false; the only test with isUnlisted: true (line 73) checks tracking. A dark-on-dark regression in textColor would pass CI.
HTML.tests.tsx covers the prop in isolation, so the gap is the wiring — an isUnlisted: true case asserting the bio text and the Read More link render mono0 would close it, mirroring what HTML.tests.tsx already does with toHaveStyle({ color: "#FFFFFF" }).
Also missing: expand/collapse for this component. Biography.tests.tsx has both directions; here fireEvent.press on Read More only checks the tracking payload, not that the full text appears or that Read Less re-truncates.
ReviewSummarySwitches the Artwork screen's
Checked and clear: I could not run Issues FoundNothing blocking. 🟡 Test gap on the inverted variant — 🟢 🟢 Doc comment overstates the closing-tag behavior — 🟢 Unneeded ternary — 🟢 Areas ReviewedBugs & edge cases. One behavior worth knowing about, not worth changing: Entity handling reads correct — Performance. The scan breaks at the budget, so it's bounded by ~140 visible chars plus markup rather than the full bio. No memoization needed. Architecture. Questions for Author
Screenshots on both platforms and the LLM assistance disclosure are both present. |
|
Closing in favor of #13851 |
2 participants
This PR resolves DI-346
Assisted-by: Claude:Opus-5
Assisted-by: Claude:Sonnet-5
Description
Fixes a markdown rendering issue on Artwork screen artist bios by updating it to follow the established pattern from the Artist screen.
This is what you see when you expand the collapsed artist bio via the "Read more" link.
There are two variants to handle: listed works (the vast majority) and unlisted works (small number, but with inverted rendering — TIL)
Listed works
The common case…
Unlisted works
Turns out that private aka unlisted works render with this component inverted.
I would have easily missed this, thanks Claude 😅
PR Checklist
To the reviewers 👀
Changelog updates
Changelog updates
Cross-platform user-facing changes
iOS user-facing changes
Android user-facing changes
Dev changes
Need help with something? Have a look at our docs, or get in touch with us.