Fixed verse ref reporting wrong when inside a char marker#462
Fixed verse ref reporting wrong when inside a char marker#462tjcouch-sil merged 3 commits intomainfrom
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders Open Preview |
ca8f61b to
b03e8dc
Compare
irahopkinson
left a comment
There was a problem hiding this comment.
Looking good, thanks for finding and fixing (much clearer code too). Just a couple of blockers.
@irahopkinson reviewed 5 files and all commit messages, and made 6 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on devin-ai-integration[bot] and tjcouch-sil).
demos/platform/src/app/app.tsx line 502 at r2 (raw file):
/> ) : ( <div className="tw-items-center tw-text-primary">
FYI thanks for fixing up the demo app after recent changes. We could also add in an external markers (backslash) menu now.
libs/shared-react/src/nodes/usj/node-react-utils.test.ts line 297 at r2 (raw file):
const v2 = $createImmutableVerseNode("2"); const t1 = $createTextNode("text1"); const c1 = $createCharNode("add");
NIT I found it a bit confusing at first as I typically use c1 to designate chapter 1 (more recently I've change to using ch1 for chapters because of this). Typically I've used char1 here. However if you see my next comment this confusion will go away.
Code quote:
c1libs/shared-react/src/nodes/usj/node-react-utils.test.ts line 298 at r2 (raw file):
const t1 = $createTextNode("text1"); const c1 = $createCharNode("add"); const t2 = $createTextNode("text2");
BTW this is an older test file from before I settled on a better pattern. I found that inline-ing all these consts made it a lot clearer to see the structure and typically the above comment wasn't even needed.
Code quote:
const root = $getRoot();
const p1 = $createParaNode();
const v1 = $createImmutableVerseNode("1");
const v2 = $createImmutableVerseNode("2");
const t1 = $createTextNode("text1");
const c1 = $createCharNode("add");
const t2 = $createTextNode("text2");libs/shared-react/src/nodes/usj/node-react.utils.ts line 441 at r2 (raw file):
* @returns the nearest previous node, or `null` if none exists. */ function $findNearestPreviousNode(node: LexicalNode): LexicalNode | undefined {
This belongs in libs/shared/src/nodes/usj/node.utils.ts since it doesn't rely on any React-specific nodes. Please move it.
tjcouch-sil
left a comment
There was a problem hiding this comment.
Thanks for the review!
@tjcouch-sil made 5 comments and resolved 1 discussion.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on irahopkinson).
libs/shared-react/src/nodes/usj/node-react-utils.test.ts line 297 at r2 (raw file):
Previously, irahopkinson (Ira Hopkinson) wrote…
NIT I found it a bit confusing at first as I typically use
c1to designate chapter 1 (more recently I've change to usingch1for chapters because of this). Typically I've usedchar1here. However if you see my next comment this confusion will go away.
Got it. Removed
libs/shared-react/src/nodes/usj/node-react-utils.test.ts line 298 at r2 (raw file):
Previously, irahopkinson (Ira Hopkinson) wrote…
BTW this is an older test file from before I settled on a better pattern. I found that inline-ing all these
consts made it a lot clearer to see the structure and typically the above comment wasn't even needed.
Definitely see why you like the inline syntax much better! AI seemed to fix it all without a problem. I checked everything and ran tests, and it all seems good. Let me know if you see anything concerning.
libs/shared-react/src/nodes/usj/node-react.utils.ts line 441 at r2 (raw file):
Previously, irahopkinson (Ira Hopkinson) wrote…
This belongs in
libs/shared/src/nodes/usj/node.utils.tssince it doesn't rely on any React-specific nodes. Please move it.
Moved :)
irahopkinson
left a comment
There was a problem hiding this comment.
@irahopkinson reviewed 3 files and all commit messages, made 2 comments, and resolved 3 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved.
libs/shared-react/src/nodes/usj/node-react-utils.test.ts line 298 at r2 (raw file):
Previously, tjcouch-sil (TJ Couch) wrote…
Definitely see why you like the inline syntax much better! AI seemed to fix it all without a problem. I checked everything and ran tests, and it all seems good. Let me know if you see anything concerning.
Thanks for fixing the whole file rather than just your changes! Particularly since AI typically follows what is already there.
The verse ref was being set to a verse in the previous paragraph because it would look at the text inside the char marker, see there was no verse, then jump to the top-level paragraph's previous sibling.
Also did a few other cleanups:
.claude/settings.local.jsonbecause claude says it's supposed to be ignoredThis change is