-
Notifications
You must be signed in to change notification settings - Fork 0
Refactor drafting UI #108
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
Refactor drafting UI #108
Changes from 7 commits
793dbfb
ef3fda1
f507dfd
a2d9029
ac9aef0
86f568f
ab82a99
209fbab
a2f5374
cc12390
40e4022
66ee340
33b1fc5
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 |
|---|---|---|
| @@ -1,17 +1,14 @@ | ||
| import { useCallback, useEffect, useRef, useState } from 'react'; | ||
| import { useCallback, useEffect, useLayoutEffect, useRef, useState } from 'react'; | ||
|
|
||
| import { useMatch, useNavigate } from '@tanstack/react-router'; | ||
| import { Loader } from 'lucide-react'; | ||
|
|
||
| import { Button } from '@/components/ui/button'; | ||
| import { useAddTranslatedVerse, useSubmitChapter } from '@/hooks/useBibleTarget'; | ||
| import { useBibleTextDebounce } from '@/hooks/useBibleTextDebounce'; | ||
| import { TargetPanel } from '@/layouts/bible/TargetPanel'; | ||
|
kaseywright marked this conversation as resolved.
|
||
| import { type ProjectItem, type User } from '@/lib/types'; | ||
| import { useAppStore } from '@/store/store'; | ||
|
|
||
| import { SourcePanel } from './SourcePanel'; | ||
|
|
||
| export interface Source { | ||
| id: number; | ||
| verseNumber: number; | ||
|
|
@@ -44,11 +41,13 @@ const DraftingUI: React.FC<DraftingUIProps> = ({ | |
| const [verses, setVerses] = useState<TargetVerse[]>(targetVerses); | ||
| const [activeVerseId, setActiveVerseId] = useState(1); | ||
| const [previousActiveVerseId, setPreviousActiveVerseId] = useState<number | null>(null); | ||
| const [textareaHeights, setTextareaHeights] = useState<Record<number, number>>({}); | ||
| const [revealedVerses, setRevealedVerses] = useState<Set<number>>(new Set()); | ||
|
|
||
| const sourceScrollRef = useRef<HTMLDivElement>(null); | ||
| const targetScrollRef = useRef<HTMLDivElement>(null); | ||
| const isScrollingSyncRef = useRef(false); | ||
|
|
||
| const textareaRefs = useRef<Record<number, HTMLTextAreaElement | null>>({}); | ||
| const verseRefs = useRef<Record<number, HTMLDivElement | null>>({}); | ||
| const [buttonTop, setButtonTop] = useState<number>(0); | ||
|
|
||
| const saveVerse = useCallback( | ||
| async (verse: number, text: string) => { | ||
|
|
@@ -76,17 +75,6 @@ const DraftingUI: React.FC<DraftingUIProps> = ({ | |
| } | ||
| ); | ||
|
|
||
| const handleHeightChange = (verseId: number, height: number) => { | ||
| setTextareaHeights(prev => { | ||
| const currentHeight = prev[verseId] || 0; | ||
| const newHeight = Math.max(currentHeight, height); | ||
| if (newHeight !== currentHeight) { | ||
| return { ...prev, [verseId]: newHeight }; | ||
| } | ||
| return prev; | ||
| }); | ||
| }; | ||
|
|
||
| useEffect(() => { | ||
| if (targetVerses.length > 0) { | ||
| targetVerses.forEach(verse => { | ||
|
|
@@ -98,31 +86,122 @@ const DraftingUI: React.FC<DraftingUIProps> = ({ | |
| return targetVerse && targetVerse.content.trim() !== ''; | ||
| }); | ||
|
|
||
| let mostRecentlyEditedVerseNumber = 1; | ||
|
|
||
| if (allVersesCompleted) { | ||
| setActiveVerseId(1); | ||
| setActiveVerseId(mostRecentlyEditedVerseNumber); | ||
| } else { | ||
| // Find the most recently edited verse (last verse with content) | ||
| let mostRecentlyEditedVerse = 1; | ||
|
|
||
| for (let i = targetVerses.length - 1; i >= 0; i--) { | ||
| if (targetVerses[i].content.trim() !== '') { | ||
| mostRecentlyEditedVerse = targetVerses[i].verseNumber; | ||
| break; | ||
| // Find the first empty verse | ||
| const firstEmptyVerse = targetVerses.find(v => v.content.trim() === '') ?? targetVerses[0]; | ||
| mostRecentlyEditedVerseNumber = firstEmptyVerse.verseNumber; | ||
| setActiveVerseId(mostRecentlyEditedVerseNumber); | ||
|
|
||
| // Scroll the active verse into view | ||
| if (mostRecentlyEditedVerseNumber > 1) { | ||
| const verseDiv = verseRefs.current[mostRecentlyEditedVerseNumber]; | ||
| if (verseDiv) { | ||
| verseDiv.scrollIntoView({ behavior: 'smooth', block: 'center' }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // If no verses have content, find first empty verse | ||
| if (mostRecentlyEditedVerse === 1 && targetVerses[0]?.content.trim() === '') { | ||
| const firstEmpty = targetVerses.find(v => v.content.trim() === ''); | ||
| if (firstEmpty) { | ||
| mostRecentlyEditedVerse = firstEmpty.verseNumber; | ||
| } | ||
| const lastVerseWithContent = (() => { | ||
| for (let i = targetVerses.length - 1; i >= 0; i--) { | ||
| if (targetVerses[i].content.trim() !== '') return targetVerses[i]; | ||
| } | ||
| setActiveVerseId(mostRecentlyEditedVerse); | ||
| } | ||
| return targetVerses[0]; | ||
| })(); | ||
|
|
||
| // Initialize revealed verses with those that have content and the initial active verse | ||
| const initiallyRevealed = new Set<number>(); | ||
| targetVerses.forEach(v => { | ||
| if (v.verseNumber <= lastVerseWithContent.verseNumber) initiallyRevealed.add(v.verseNumber); | ||
| }); | ||
| initiallyRevealed.add(mostRecentlyEditedVerseNumber); | ||
| setRevealedVerses(initiallyRevealed); | ||
| } | ||
| }, [targetVerses, sourceVerses, setInitialContent]); | ||
|
|
||
| // Whenever active verse changes, mark it as revealed | ||
| useEffect(() => { | ||
| setRevealedVerses(prev => { | ||
| if (prev.has(activeVerseId)) return prev; | ||
| const next = new Set(prev); | ||
| next.add(activeVerseId); | ||
| return next; | ||
| }); | ||
| }, [activeVerseId]); | ||
|
|
||
| // Ensure revealed textareas are properly sized on reveal (including on page load) | ||
| useEffect(() => { | ||
| revealedVerses.forEach(verseNumber => { | ||
| const textarea = textareaRefs.current[verseNumber]; | ||
| if (textarea) autoResizeTextarea(textarea); | ||
| }); | ||
| }, [revealedVerses]); | ||
|
|
||
| useEffect(() => { | ||
| if (verses.length === 0) { | ||
| setVerses([ | ||
| { | ||
| verseNumber: 1, | ||
| content: '', | ||
| }, | ||
| ]); | ||
| } else { | ||
| verses.forEach(verse => { | ||
| const textarea = textareaRefs.current[verse.verseNumber]; | ||
| if (textarea && verse.content) { | ||
| autoResizeTextarea(textarea); | ||
| } | ||
| }); | ||
| } | ||
| }, [setVerses, verses]); | ||
|
|
||
| const autoResizeTextarea = (textarea: HTMLTextAreaElement) => { | ||
| textarea.style.height = 'auto'; | ||
| textarea.style.height = Math.max(20, textarea.scrollHeight) + 'px'; | ||
| }; | ||
|
|
||
| const updateButtonPosition = useCallback(() => { | ||
| const container = targetScrollRef.current; | ||
| const textarea = textareaRefs.current[activeVerseId]; | ||
| if (!container || !textarea) return; | ||
|
|
||
| const containerRect = container.getBoundingClientRect(); | ||
| const textareaRect = textarea.getBoundingClientRect(); | ||
| const top = container.scrollTop + (textareaRect.bottom - containerRect.top) + 20; // 20px gap | ||
| setButtonTop(top); | ||
| }, [activeVerseId]); | ||
|
|
||
| const scrollVerseToTop = useCallback((verseNumber: number) => { | ||
| const container = targetScrollRef.current; | ||
| const row = verseRefs.current[verseNumber]; | ||
| if (!container || !row) return; | ||
|
|
||
| const containerRect = container.getBoundingClientRect(); | ||
| const rowRect = row.getBoundingClientRect(); | ||
|
|
||
| // Compute new scrollTop so that the row's top aligns with the container's top | ||
| const newScrollTop = container.scrollTop + (rowRect.top - containerRect.top); | ||
| container.scrollTo({ top: newScrollTop, behavior: 'smooth' }); | ||
| }, []); | ||
|
|
||
| useLayoutEffect(() => { | ||
| // Focus and position updates after DOM mutations, before paint | ||
| const textarea = textareaRefs.current[activeVerseId]; | ||
| if (textarea) { | ||
| textarea.focus(); | ||
| const len = textarea.value.length; | ||
| try { | ||
| textarea.setSelectionRange(len, len); | ||
| } catch {} | ||
| autoResizeTextarea(textarea); | ||
| } | ||
| updateButtonPosition(); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
|
kaseywright marked this conversation as resolved.
Outdated
|
||
| }, [activeVerseId, verses]); | ||
|
|
||
| const totalSourceVerses = sourceVerses.length; | ||
| const versesWithText = verses.filter(v => v.content.trim() !== '').length; | ||
| const countWithContent = verses.filter(v => v.content && v.content.trim() !== '').length; | ||
|
|
@@ -146,6 +225,16 @@ const DraftingUI: React.FC<DraftingUIProps> = ({ | |
| debouncedSave(id, text); | ||
| }; | ||
|
|
||
| const handleTextChange = (verseId: number, text: string) => { | ||
| updateTargetVerse(verseId, text); | ||
|
|
||
| const textarea = textareaRefs.current[verseId]; | ||
| if (textarea) { | ||
| autoResizeTextarea(textarea); | ||
| } | ||
| updateButtonPosition(); | ||
| }; | ||
|
|
||
| const handleActiveVerseChange = async (newVerseId: number) => { | ||
| if (previousActiveVerseId !== null && previousActiveVerseId !== newVerseId) { | ||
| const previousVerse = verses.find(v => v.verseNumber === previousActiveVerseId); | ||
|
|
@@ -159,6 +248,10 @@ const DraftingUI: React.FC<DraftingUIProps> = ({ | |
|
|
||
| setPreviousActiveVerseId(activeVerseId); | ||
| setActiveVerseId(newVerseId); | ||
| // useLayoutEffect will focus and reposition Next button after state updates | ||
| // Also scroll so the previous verse aligns to the top (or verse 1) | ||
| const prevId = Math.max(1, newVerseId - 1); | ||
| requestAnimationFrame(() => scrollVerseToTop(prevId)); | ||
| }; | ||
|
|
||
| const moveToNextVerse = useCallback(async () => { | ||
|
|
@@ -182,23 +275,19 @@ const DraftingUI: React.FC<DraftingUIProps> = ({ | |
| if (status.hasUnsavedChanges) { | ||
| await saveImmediately(activeVerseId, currentVerse.content); | ||
| } | ||
| // Scroll so the previous verse aligns to the top of the container (or verse 1) | ||
| const prevId = Math.max(1, nextVerseId - 1); | ||
| requestAnimationFrame(() => scrollVerseToTop(prevId)); | ||
| } | ||
| }, [activeVerseId, verses, totalSourceVerses, saveImmediately, setInitialContent, getSaveStatus]); | ||
|
|
||
| const handleScroll = (source: 'source' | 'target', scrollTop: number) => { | ||
| if (isScrollingSyncRef.current) return; | ||
| isScrollingSyncRef.current = true; | ||
|
|
||
| if (source === 'source' && targetScrollRef.current) { | ||
| targetScrollRef.current.scrollTop = scrollTop; | ||
| } else if (source === 'target' && sourceScrollRef.current) { | ||
| sourceScrollRef.current.scrollTop = scrollTop; | ||
| } | ||
|
|
||
| setTimeout(() => { | ||
| isScrollingSyncRef.current = false; | ||
| }, 50); | ||
| }; | ||
| }, [ | ||
| activeVerseId, | ||
| verses, | ||
| totalSourceVerses, | ||
| saveImmediately, | ||
| setInitialContent, | ||
| getSaveStatus, | ||
| scrollVerseToTop, | ||
| ]); | ||
|
|
||
| const handleSubmit = async () => { | ||
| if (isTranslationComplete) { | ||
|
|
@@ -219,6 +308,13 @@ const DraftingUI: React.FC<DraftingUIProps> = ({ | |
| } | ||
| }; | ||
|
|
||
| const handleKeyDown = async (e: React.KeyboardEvent) => { | ||
| if (e.key === 'Enter' && !e.shiftKey) { | ||
| e.preventDefault(); | ||
| await moveToNextVerse(); | ||
| } | ||
| }; | ||
|
|
||
| return ( | ||
| <div className='flex h-full flex-col overflow-hidden'> | ||
| <div className='flex-shrink-0'> | ||
|
|
@@ -258,34 +354,89 @@ const DraftingUI: React.FC<DraftingUIProps> = ({ | |
| </div> | ||
| </div> | ||
|
|
||
| <div className='flex-1 overflow-hidden'> | ||
| <div className='flex h-full'> | ||
| <div className='w-1/2'> | ||
| <SourcePanel | ||
| activeVerseId={activeVerseId} | ||
| bibleName={projectItem.bibleName} | ||
| scrollRef={sourceScrollRef} | ||
| textareaHeights={textareaHeights} | ||
| verses={sourceVerses} | ||
| onHeightChange={handleHeightChange} | ||
| onScroll={scrollTop => handleScroll('source', scrollTop)} | ||
| /> | ||
| <div className='mx-auto max-w-5xl flex-1 overflow-hidden'> | ||
| <div className='grid h-full grid-cols-2' style={{ gridTemplateRows: '4rem 1fr' }}> | ||
| <div className='bg-background sticky top-0 z-10 ml-8 px-6 py-4'> | ||
| <h3 className='text-xl font-bold text-gray-800'>{projectItem.bibleName}</h3> | ||
| </div> | ||
| <div className='bg-background sticky top-0 z-10 py-4'> | ||
| <h3 className='text-xl font-bold text-gray-800'>{projectItem.targetLanguage}</h3> | ||
| </div> | ||
| <div className='w-1/2'> | ||
| <TargetPanel | ||
| activeVerseId={activeVerseId} | ||
| moveToNextVerse={moveToNextVerse} | ||
| scrollRef={targetScrollRef} | ||
| setActiveVerseId={handleActiveVerseChange} | ||
| setVerses={setVerses} | ||
| targetLanguage={projectItem.targetLanguage} | ||
| textareaHeights={textareaHeights} | ||
| totalSourceVerses={totalSourceVerses} | ||
| updateVerse={updateTargetVerse} | ||
| verses={verses} | ||
| onHeightChange={handleHeightChange} | ||
| onScroll={scrollTop => handleScroll('target', scrollTop)} | ||
| /> | ||
|
|
||
| <div | ||
| ref={targetScrollRef} | ||
| className='relative col-span-2 flex h-full flex-col overflow-y-auto' | ||
| onScroll={() => updateButtonPosition()} | ||
| > | ||
| {sourceVerses.map(verse => { | ||
| const isActive = activeVerseId === verse.verseNumber; | ||
| const currentTargetVerse = verses.find(v => v.verseNumber === verse.verseNumber); | ||
| return ( | ||
| <div | ||
| key={verse.verseNumber} | ||
| ref={el => (verseRefs.current[verse.verseNumber] = el)} | ||
| className='grid grid-cols-2 gap-4 px-6 py-4' | ||
| > | ||
| {/* source verse */} | ||
| <div className='col-1 flex items-start transition-all'> | ||
| <div className='w-8 flex-shrink-0'> | ||
| <span className='text-lg font-medium text-gray-700'>{verse.verseNumber}</span> | ||
| </div> | ||
| <div className='flex-1'> | ||
| <div | ||
| className={`bg-card rounded-lg border border-2 px-4 py-1 shadow-sm transition-all ${isActive ? 'border-primary' : ''}`} | ||
| > | ||
| <p className='min-h-12 content-center overflow-hidden text-base leading-relaxed leading-snug text-gray-800 outline-none'> | ||
| {verse.text} | ||
| </p> | ||
| </div> | ||
| </div> | ||
| </div> | ||
|
|
||
| {/* target verse */} | ||
| <div | ||
| className={`col-2 flex transition-all ${isActive || revealedVerses.has(verse.verseNumber) ? '' : 'hidden'}`} | ||
| > | ||
| <div | ||
| className={`flex-1 cursor-pointer rounded-lg border border-2 px-4 py-1 shadow-sm transition-all ${isActive ? 'border-primary' : ''}`} | ||
| onClick={() => handleActiveVerseChange(verse.verseNumber)} | ||
| > | ||
| <textarea | ||
|
kaseywright marked this conversation as resolved.
|
||
| ref={el => (textareaRefs.current[verse.verseNumber] = el)} | ||
| aria-label={`Translation for verse ${verse.verseNumber}`} | ||
| autoCapitalize='sentences' | ||
| autoCorrect='on' | ||
| className='h-auto min-h-3 w-full resize-none content-center overflow-hidden border-none bg-transparent text-base leading-relaxed leading-snug text-gray-800 outline-none' | ||
| id={`verse-${verse.verseNumber}`} | ||
| placeholder='Enter translation...' | ||
| spellCheck={true} | ||
| value={currentTargetVerse?.content ?? ''} | ||
| onChange={e => handleTextChange(verse.verseNumber, e.target.value)} | ||
| onFocus={() => handleActiveVerseChange(verse.verseNumber)} | ||
| onKeyDown={e => handleKeyDown(e)} | ||
| /> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| ); | ||
| })} | ||
|
|
||
| {activeVerseId < totalSourceVerses && ( | ||
| <div className='absolute right-4 z-10' style={{ top: buttonTop }}> | ||
| <Button | ||
|
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.
Contributor
Author
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. 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. After discussing this, we have decided to keep the Next Verse button below the last verse that can be edited. The Next Verse button will be used only for "unlocking" the next verse for editing. The Enter or Tab keys will move the user down a verse. So, for example, if they translate all the way to verse 6 and then see a needed edit in verse 3, they can click on verse 3, make the edit, then press Enter three times to be back in the edit box for verse 6 and continue translating. If they hit Enter again when they are in the edit box for verse 6 or if they click the Next Verse button, it will move them to verse 7. |
||
| className={`bg-primary flex items-center gap-2 px-6 py-2 font-medium shadow-lg transition-all ${ | ||
| verses.find(v => v.verseNumber === activeVerseId)?.content.trim() | ||
| ? 'hover:bg-primary-hover cursor-pointer text-white' | ||
| : 'cursor-not-allowed bg-gray-300 text-gray-500' | ||
| }`} | ||
| disabled={!verses.find(v => v.verseNumber === activeVerseId)?.content.trim()} | ||
| title='Next Verse (Enter)' | ||
| onClick={() => moveToNextVerse()} | ||
| > | ||
| Next Verse | ||
| </Button> | ||
| </div> | ||
| )} | ||
| </div> | ||
| </div> | ||
| </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.
The content should be stretched to use more horizontal space. There's space on both sides.
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.
There is a well understood design principle that text that is too wide is more difficult to read. The best width for text should be between 50-75 characters, or for blogs or larger bodies of text, the text area should be no more than roughly 700px. ref
The width I set is generally closer to 55 characters, depending on language. I can see an argument for a little wider text area here ( maybe up to
max-w-7xl) but not filling the screen on larger devices.Uh oh!
There was an error while loading. Please reload this page.
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.
I have always thought, like Kasey, that shorter line lengths are better. But I have done some research and I am finding that the shorter lengths apply to reading and not editing. For editing, having a larger context is better. Taking from other programs like Google Docs or MS Word they seem to be about 95 characters wide. I think Kasey's recommendation of 700px (or the widest preset width in Tailwind) for each the source and target pane will allow for that.