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

Jump to bottom

Fix layout of nested blockquotes #202

Merged
merged 9 commits into from
Feb 23, 2024
Merged

Fix layout of nested blockquotes #202

merged 9 commits into from
Feb 23, 2024

Conversation

robertKozik
Copy link
Collaborator

Details

Related Issues

GH_LINK

Manual Tests

Linked PRs

tomekzaw
tomekzaw reviewed Feb 23, 2024
View reviewed changes
android/src/main/java/com/expensify/livemarkdown/MarkdownBlockquoteSpan.java Outdated
Comment on lines 42 to 44
float shift = (marginLeft + borderWidth + paddingLeft) * level;
float left = x + dir * marginLeft + shift;
float right = x + dir * (marginLeft + borderWidth) + shift;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we multiply marginLeft by dir, shouldn't we also multiply shift by dir?

parser/index.ts
Comment on lines +217 to +218
const groupedRanges = groupRanges(sortedRanges);
return groupedRanges;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add some unit tests for nested blockquotes to verify that groupRanges indeed works correctly?

src/web/parserUtils.ts Outdated
if (!range.depth) {
ungroupedRanges.push(range);
}
Array.from({length: range.depth!}).forEach(() => ungroupedRanges.push(range));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we remove depth from pushed ranges here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are doing it before parsing, there is no need to do so, the main loop of the parser is not using the info about depth

Copy link
Collaborator

@tomekzaw tomekzaw Feb 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand, it's just that it could be confusing to still see depth in there, like:

[
  {style: "blockquote", start: 0, end: 10, depth: 3},
  {style: "blockquote", start: 0, end: 10, depth: 3},
  {style: "blockquote", start: 0, end: 10, depth: 3},
]

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I'll add removing it

src/web/parserUtils.ts Outdated
@@ -87,7 +98,7 @@ function parseRangesToHTMLNodes(text: string, ranges: MarkdownRange[], markdownS
return root;
}

const stack = [...ranges];
const stack = [...ungroupRanges(ranges)];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to make an additional copy here?

Suggested change
const stack = [...ungroupRanges(ranges)];
const stack = ungroupRanges(ranges);

tomekzaw
tomekzaw reviewed Feb 23, 2024
View reviewed changes
parser/__tests__/index.test.js Outdated
@@ -332,4 +332,42 @@ describe('trailing whitespace', () => {
]);
});
});

describe('nested quotes', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use "blockquote" instead of "quote" consistently across this file, let's stick to that

Suggested change
describe('nested quotes', () => {
describe('nested blockquotes', () => {

parser/__tests__/index.test.js Outdated
]);
});

test('two separate quote depths', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
test('two separate quote depths', () => {
test('different blockquote depths', () => {

parser/__tests__/index.test.js Outdated
});

test('two separate quote depths', () => {
expect('>> Hello 1 \n > Hello 2').toBeParsedAs([
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
expect('>> Hello 1 \n > Hello 2').toBeParsedAs([
expect('>> Hello 1\n> Hello 2').toBeParsedAs([

parser/__tests__/index.test.js Outdated
]);
});

test('with diffrent markdown inside', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
test('with diffrent markdown inside', () => {
test('with another style inside', () => {

parser/__tests__/index.test.js Outdated
Comment on lines 338 to 339
expect('>> Hello world').toBeParsedAs([
{type: 'blockquote', start: 0, length: 14, depth: 2},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
expect('>> Hello world').toBeParsedAs([
{type: 'blockquote', start: 0, length: 14, depth: 2},
expect('>>> Hello world').toBeParsedAs([
{type: 'blockquote', start: 0, length: 14, depth: 3},

parser/__tests__/index.test.js Outdated
@@ -332,4 +332,42 @@ describe('trailing whitespace', () => {
]);
});
});

describe('nested quotes', () => {
test('simple example', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make this test consistent to the following one

Suggested change
test('simple example', () => {
test('without whitespace between syntax', () => {

@robertKozik @tomekzaw
correct dir multiplication
746a8a3 
Co-authored-by: Tomek Zawadzki <[email protected]>
@tomekzaw tomekzaw changed the title Support grouping overlapping ranges Fix layout of nested blockquotes Feb 23, 2024
This was referenced Feb 23, 2024
Closed
Closed
tomekzaw
tomekzaw approved these changes Feb 23, 2024
View reviewed changes
Copy link
Collaborator

@tomekzaw tomekzaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on Android, iOS and web. Thanks a lot for your efforts on it! ❤️

This was linked to issues Feb 23, 2024
Show more vertical lines for nested blockquotes #89
Closed
Shifted cursor position for nested multiline blockquotes on Android #90
Closed
@robertKozik robertKozik merged commit cf8a2ff into main Feb 23, 2024
2 checks passed
@robertKozik robertKozik deleted the support-grouping-ranges branch February 23, 2024 13:08
@robertKozik robertKozik mentioned this pull request Feb 23, 2024
Closed
@tomekzaw tomekzaw mentioned this pull request Feb 26, 2024
Merged
@tomekzaw tomekzaw mentioned this pull request Mar 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomekzaw tomekzaw tomekzaw approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@robertKozik @tomekzaw