YPE-953: fix(ui): fix/refactor the footnotes logic #94
Conversation
bmanquen
commented
Jan 6, 2026
bmanquen
commented
- sanitize html before parsing/transforming the string
- use React Portals instead of React Root
- update popover for the footnotes to use new popover structure
🦋 Changeset detectedLatest commit: 8807d08 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 |
|
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 SummaryThis PR refactors the footnotes rendering logic to fix persistence issues and improve code architecture. Key Changes:
Benefits:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Verse.Html
participant yvDomTransformer
participant DOMPurify
participant extractNotesFromHtml
participant HtmlWithNotes
participant Portal as React Portal
participant DOM
participant VerseFootnoteButton
User->>Verse.Html: Render with HTML content
Verse.Html->>yvDomTransformer: Transform HTML with extractNotes=true
yvDomTransformer->>extractNotesFromHtml: Extract footnotes
extractNotesFromHtml->>DOMPurify: Sanitize HTML first
DOMPurify-->>extractNotesFromHtml: Sanitized HTML
extractNotesFromHtml->>extractNotesFromHtml: Parse DOM and extract notes
extractNotesFromHtml->>extractNotesFromHtml: Create placeholders [data-verse-footnote]
extractNotesFromHtml-->>yvDomTransformer: {html, notes}
yvDomTransformer->>DOMPurify: Sanitize transformed HTML
yvDomTransformer->>yvDomTransformer: Add non-breaking spaces
yvDomTransformer-->>Verse.Html: {html, notes}
Verse.Html->>HtmlWithNotes: Render with html & notes
HtmlWithNotes->>DOM: Set innerHTML with html
HtmlWithNotes->>HtmlWithNotes: Query placeholders
HtmlWithNotes->>HtmlWithNotes: Store placeholders in state
loop For each placeholder
HtmlWithNotes->>Portal: createPortal(VerseFootnoteButton, placeholder)
Portal->>DOM: Render button into placeholder
end
User->>VerseFootnoteButton: Click footnote button
VerseFootnoteButton->>VerseFootnoteButton: Open Popover with notes
|
cameronapak
changed the title
- the dom was getting re-rendered multiple times especially when adding the bible reader settings. - this change allows the verse text to only be re-rendered when a different html string is passed in explicitly.
- uses new props on our popover component to reduce redundancy in the header of the popover.
suggestion (non-blocking): There's a way to better align the footnotes so that the line height of the sentence that it's in, or the paragraph that it's in, doesn't get messed up because of the footnote. This isn't something that you need to do in this at all. I just wanted to share that I found a simple way to fix it. On the component that has {
display: inline-flex;
vertical-align: top;
margin-left: 0.25rem;
}
|
cameronapak
left a comment
cameronapak
left a comment
There was a problem hiding this comment.
praise: The code itself looks good and the tests are clean. I've never used React portals yet, so this was a fun thing to see and learn a little bit about.
question: Do you feel that this is stable and that footnotes are in a good spot?
bmanquen
force-pushed
the
bm/YPE-953-refactor-footnotes
branch
from
f47c185 to
a0772bb
Compare
|
question: @greptileai, my teammate did a great job getting this working and working well. I was wondering, with the recent Web Popover API stuff, is that something that could replace this React-based approach and simplify things? I'm asking just for an understanding, not for exact technical implementation details or to change this task. |
|
Looking at this PR that's implementing footnotes with React Portals and a popover structure, that's a great question about the Web Popover API! The Web Popover API could potentially simplify this approach, but there are some important considerations: Where it could help:
Current limitations:
For footnotes specifically, the native popover API could be really nice since they're typically simple content that appears on hover/click with straightforward positioning needs. Your teammate's React Portal approach gives you full control and guaranteed compatibility across all browsers right now, while the Web Popover API could be a future simplification once browser support is more complete. It's definitely worth keeping an eye on as the API matures! |
Yes I think it is now much more stable after I make your suggested CSS change! |
cameronapak
left a comment
cameronapak
left a comment
There was a problem hiding this comment.
Thank you for submitting the changeset. This is approved.
|
@greptileai thanks for your response #94 (comment) Since the HTML comes back as a string, we can modify the string. Or we can use a virtual DOM or something to modify the string, to modify the element so that it has the popover API information stuff that it needs. Let's say we did this approach, and we did a more web standards first approach and not take out the React complexity of this feature because it starts out as HTML and then would be rendered as HTML. How would this change the file? Like how would this change the footnotes file? Would it make the code a lot simpler? Would it still do the same things? |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looking at the current implementation, using the native Popover API would indeed simplify the code significantly. Here's how it would change:
Current Complexity
The current approach:
- Parses HTML to extract footnote data
- Uses React Portals to render React components into DOM placeholders
- Manages React state and lifecycle for popovers
- Requires complex coordination between HTML parsing and React rendering
Web Standards Approach
With the Popover API, you'd modify the HTML string directly to add the necessary attributes and content:
'use client';
import { useLayoutEffect, forwardRef, useRef, type ReactNode } from 'react';
import DOMPurify from 'isomorphic-dompurify';
import { usePassage } from '@youversion/platform-react-hooks';
const NON_BREAKING_SPACE = '\u00A0';
type VerseProps = {
html?: string;
extractNotes?: boolean;
reference?: string;
className?: string;
children?: ReactNode;
};
function transformFootnotesToPopoverAPI(html: string): string {
if (typeof window === 'undefined') return html;
const sanitizedHtml = DOMPurify.sanitize(html, DOMPURIFY_CONFIG);
const parser = new DOMParser();
const doc = parser.parseFromString(sanitizedHtml, 'text/html');
const noteElements = doc.querySelectorAll('span.yv-n.f');
noteElements.forEach((noteEl, index) => {
const footnoteId = `footnote-${index}`;
const popoverId = `popover-${index}`;
// Get the verse number from context
const verseEl = noteEl.closest('[data-verse]') ||
noteEl.parentElement?.querySelector('.yv-v[v]');
const verseNum = verseEl?.getAttribute('v') || '';
// Create footnote button with popover trigger
const button = doc.createElement('button');
button.className = 'yv:inline-flex yv:items-center yv:justify-center yv:w-4 yv:h-4 yv:text-xs yv:bg-blue-100 yv:text-blue-700 yv:rounded-full yv:ml-1 yv:align-super hover:yv:bg-blue-200';
button.setAttribute('popovertarget', popoverId);
button.setAttribute('data-verse-footnote', verseNum);
button.innerHTML = '<svg width="12" height="12" viewBox="0 0 24 24" fill="currentColor"><path d="M12 2C6.48 2 2 6.48 2 12s4.48 10 10 10 10-4.48 10-10S17.52 2 12 2zm1 15h-2v-6h2v6zm0-8h-2V7h2v2z"/></svg>';
// Create popover content
const popover = doc.createElement('div');
popover.id = popoverId;
popover.setAttribute('popover', 'auto');
popover.className = 'yv:bg-white yv:border yv:rounded-lg yv:shadow-lg yv:p-4 yv:max-w-sm';
const header = doc.createElement('div');
header.className = 'yv:font-bold yv:mb-2';
header.textContent = `Footnotes - Verse ${verseNum}`;
const content = doc.createElement('div');
content.innerHTML = noteEl.innerHTML;
popover.appendChild(header);
popover.appendChild(content);
// Replace the footnote span with button and append popover to body
noteEl.replaceWith(button);
doc.body.appendChild(popover);
});
return doc.body.innerHTML;
}
function yvDomTransformer(html: string, extractNotes: boolean = false): string {
let processedHtml = html;
if (extractNotes) {
processedHtml = transformFootnotesToPopoverAPI(html);
} else {
processedHtml = DOMPurify.sanitize(html, DOMPURIFY_CONFIG);
}
// Safely parse and modify HTML to add spaces to paragraph elements
const parser = new DOMParser();
const doc = parser.parseFromString(processedHtml, 'text/html');
// Add non-breaking spaces to verse labels
const verseLabels = doc.querySelectorAll('span.yv-vlbl');
verseLabels.forEach((label) => {
if (label.textContent && !label.textContent.endsWith(NON_BREAKING_SPACE)) {
label.textContent += NON_BREAKING_SPACE;
}
});
return doc.body.innerHTML;
}
// Configure DOMPurify to allow popover attributes
const DOMPURIFY_CONFIG: DOMPurify.Config = {
ALLOWED_TAGS: [
'div', 'span', 'p', 'br', 'strong', 'em', 'i', 'b', 'u', 's', 'sup', 'sub', 'small',
'h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'ul', 'ol', 'li', 'blockquote', 'cite', 'q',
'abbr', 'acronym', 'time', 'code', 'pre', 'kbd', 'samp', 'var', 'mark', 'ins', 'del',
'button', // For footnote buttons
],
ALLOWED_ATTR: [
'class', 'id', 'title', 'lang', 'dir', 'style', 'data-*', 'v', 'usfm',
'popover', 'popovertarget', // Popover API attributes
],
ALLOW_DATA_ATTR: true,
};
export const Verse = forwardRef<HTMLDivElement, VerseProps>(
({ html, extractNotes = false, reference, className, children, ...props }, ref) => {
const { passage } = usePassage();
const contentRef = useRef<HTMLDivElement>(null);
const content = html || passage?.content || '';
const processedHtml = yvDomTransformer(content, extractNotes);
useLayoutEffect(() => {
if (!contentRef.current) return;
contentRef.current.innerHTML = processedHtml;
}, [processedHtml]);
return (
<div ref={ref} className={className} {...props}>
<div ref={contentRef} />
{children}
</div>
);
},
);
Verse.displayName = 'Verse';
Benefits of This Approach
- Much Simpler: No React Portals, no state management, no complex lifecycle coordination
- Web Standards: Uses native Popover API - better accessibility and performance
- Fewer Dependencies: No need for complex React popover components
- Better Performance: Native browser implementation is faster than React rendering
- Cleaner Architecture: Pure HTML transformation, no React/DOM boundary crossing
Same Functionality
- Footnotes still appear as clickable buttons
- Popovers still show footnote content
- Still sanitizes HTML for security
- Still maintains verse references and styling
The main trade-off is that you lose some React component flexibility (like custom styling via props), but gain significant simplicity and performance. The popover styling would be handled via CSS rather than React component props.
detection - Fix verse boundaries to work across paragraph elements using TreeWalker - Add `v` and `usfm` to DOMPurify allowed attributes (was stripping verse markers) - Place footnote icons at actual verse end, not next verse's paragraph - Exclude headers (.yv-h) from popover verse text - Add spacing between paragraphs in reconstructed verse text - Style footnote markers (A, B, C) with muted foreground color - Inherit reader font size in popover verse content - Remove verse number from popover verse text (shown in header already) - Add tests for footnote extraction and placement - Simplify and refactor extraction logic (~145 to ~85 lines)
bmanquen
changed the title
| it('should handle multiple footnotes in a single verse', async () => { | ||
| const htmlWithMultipleNotes = ` | ||
| <div class="p"> | ||
| <span class="yv-v" v="51"></span><span class="yv-vlbl">51</span>He then added, | ||
| <span class="wj">"Very truly I tell you,</span><span class="yv-n f"><span class="fr">1:51 </span><span class="ft">The Greek is plural.</span></span> | ||
| <span class="wj">you</span><span class="yv-n f"><span class="fr">1:51 </span><span class="ft">The Greek is plural.</span></span> | ||
| <span class="wj">will see heaven open."</span> | ||
| </div> | ||
| `; | ||
|
|
||
| const { container } = render(<Verse.Html html={htmlWithMultipleNotes} renderNotes={true} />); | ||
|
|
||
| await waitFor(() => { | ||
| const placeholder = container.querySelector('[data-verse-footnote="51"]'); | ||
| expect(placeholder).not.toBeNull(); | ||
| const footnoteElements = container.querySelectorAll('.yv-n.f'); | ||
| expect(footnoteElements.length).toBe(0); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This test doesn't seem to be asserting that there are multiple footnotes in the popover element.
| } else if ( | ||
| !node.classList.contains('yv-v') && | ||
| !node.classList.contains('yv-vlbl') && | ||
| !node.childNodes.length | ||
| ) { | ||
| text += node.textContent || ''; | ||
| } |
There was a problem hiding this comment.
What case is this else if cover? It seems like this will never get hit.
