Skip to content
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

Merged
merged 7 commits into from
Nov 15, 2024
Merged

Conversation

LeMurphant
Copy link
Contributor

Fix #64

@@ -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;
Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor

@jrhender jrhender left a 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;
Copy link
Contributor

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) {
Copy link
Contributor

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?

Copy link
Contributor Author

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
Comment on lines 303 to 306
} else {
// Add quote marker if the paragraph is indented beyond our threshold
const isQuote = indentStart >= QUOTE_INDENT_THRESHOLD;
const quotePrefix = isQuote ? "> " : "";
Copy link
Contributor

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

Suggested change
} 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.

Copy link
Contributor Author

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

Copy link
Contributor

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 👍

@LeMurphant LeMurphant marked this pull request as draft November 15, 2024 19:28
@LeMurphant LeMurphant marked this pull request as ready for review November 15, 2024 19:50
@LeMurphant LeMurphant merged commit c4f220c into main Nov 15, 2024
1 check passed
@LeMurphant LeMurphant deleted the issue-64-blockquotes-murphant branch November 15, 2024 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blockquotes not working
2 participants