-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add blockquote functionality #73
Conversation
@@ -239,7 +239,12 @@ export const mergeSameElements = (elements) => | |||
|
|||
export const parseParagraph = (documentContext) => (paragraph) => { | |||
const { elements, ...paragraphContext } = paragraph; | |||
const paragraphStyleName = paragraphContext.paragraphStyle?.namedStyleType; | |||
const paragraphStyle = paragraphContext.paragraphStyle || {}; | |||
const paragraphStyleName = paragraphStyle.namedStyleType; |
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'm not sure about this part, it was recommended by Claude
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 think this is ok. In both the previous and the new code, if paragraphContext.paragraphStyle
then paragraphStyleName
will undefined
. I think this change is just to have paragraphStyle
as a separate variable to be able to use it below.
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.
Looks good to me
@@ -239,7 +239,12 @@ export const mergeSameElements = (elements) => | |||
|
|||
export const parseParagraph = (documentContext) => (paragraph) => { | |||
const { elements, ...paragraphContext } = paragraph; | |||
const paragraphStyleName = paragraphContext.paragraphStyle?.namedStyleType; | |||
const paragraphStyle = paragraphContext.paragraphStyle || {}; | |||
const paragraphStyleName = paragraphStyle.namedStyleType; |
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 think this is ok. In both the previous and the new code, if paragraphContext.paragraphStyle
then paragraphStyleName
will undefined
. I think this change is just to have paragraphStyle
as a separate variable to be able to use it below.
@@ -250,7 +255,7 @@ export const parseParagraph = (documentContext) => (paragraph) => { | |||
let leadingSpace = ""; | |||
|
|||
// First we check if the "paragraph" is a heading, because the markdown for a heading is the first thing we need to output | |||
if (paragraphStyleName.indexOf("HEADING_") === 0) { | |||
if (paragraphStyleName?.indexOf("HEADING_") === 0) { |
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.
Is separate bug fix or does it relate to the blockquote functionality?
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.
In the previous version, the ?
was integrated directly in the declaration of the variable. This seems to be an update (recommended by Claude) to keep the same functionality. I'm surprised that namedStyleType
is not used for it, but I don't understand what this part does.
parser/parser.js
Outdated
} else { | ||
// Add quote marker if the paragraph is indented beyond our threshold | ||
const isQuote = indentStart >= QUOTE_INDENT_THRESHOLD; | ||
const quotePrefix = isQuote ? "> " : ""; |
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.
A bit of a nit so no problem if you want to keep it as is, but a consideration could be that this is introducing a nested conditional (the ternary in the else
). If text classifications are mutually exclusive (i.e. the text can either be bullet, block quote or normal) then having the conditionals in a single level chain can make this clearer, IMO.
To lift the conditional, the isQuote
logic could be moved next to where indentStart
and QUOTE_INDENT_THRESHOLD
are declared (might be good to have them all together anyways) or a small isQuote(paragraphStyle)
function could be defined.
// Add quote marker if the paragraph is indented beyond our threshold
const isQuote = indentStart >= QUOTE_INDENT_THRESHOLD;
Then the conditional would just be
} else { | |
// Add quote marker if the paragraph is indented beyond our threshold | |
const isQuote = indentStart >= QUOTE_INDENT_THRESHOLD; | |
const quotePrefix = isQuote ? "> " : ""; | |
} else if (isQuote) { | |
const quotePrefix = "> "; |
Note an else
statement with a separate return would still be necessary for normal text.
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 ended up defining quotePrefix everywhere as an empty string, and updating it if needed. LMK what you think of this type of logic
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.
Yea, current logic looks good to me 👍
Co-authored-by: John Henderson <[email protected]>
Fix #64