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.

Sign up for GitHub

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

Add blockquote functionality #73

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7c96501
Add blockquote functionality
LeMurphant Nov 13, 2024
00bafd8
Lint
LeMurphant Nov 13, 2024
37126a9
Added tests
LeMurphant Nov 13, 2024
f07f26d
Lint
LeMurphant Nov 13, 2024
b129138
Split ternary operator (partial)
LeMurphant Nov 15, 2024
a694c46
Separate isQuote into its own function
LeMurphant Nov 15, 2024
18dd9dc
Lint
LeMurphant Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions parser/parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.


// Check indentation - Google Docs API provides this in PT units
const indentStart = paragraphStyle.indentStart?.magnitude || 0;
const QUOTE_INDENT_THRESHOLD = 18; // Standard indentation button is 36pt, we test lower

let md = mergeSameElements(elements).map(
parseElement({ documentContext, paragraphContext })
Expand All @@ -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.

const headingLevel = parseInt(paragraphStyleName[8]);
const headingPrefix = new Array(headingLevel).fill("#").join("") + " ";
prefix = headingPrefix;
Expand Down Expand Up @@ -296,11 +301,15 @@ export const parseParagraph = (documentContext) => (paragraph) => {
md.join("").replaceAll("\n", "\n" + leadingSpace + " ")
);
} 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 👍

return (
leadingSpace +
itemMarker +
quotePrefix +
prefix +
md.join("").replaceAll("\n", "\n" + leadingSpace)
md.join("").replaceAll("\n", "\n" + leadingSpace + quotePrefix)
);
}
};
Expand Down
Loading