Skip to content
This repository has been archived by the owner on Feb 21, 2019. It is now read-only.

add support for blockPerLine option #7

Closed

Conversation

ianstormtaylor
Copy link

Hey @Soreine this solves #4.

It's a bit convoluted, but I tried to leave as much existing code in place as possible, while keeping the new logic not too crazy to understand.

Basically if options.blockPerLine, then the block passed into the decorator is assumed to be the outer-most block. And it tokenizes that outer-most block's text, and then applies the marks to the individual leaf text nodes.

@SamyPesse SamyPesse requested a review from Soreine October 11, 2017 07:22
Copy link
Contributor

@Soreine Soreine left a comment

Choose a reason for hiding this comment

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

How did you check that it works?
I have seen a bug in text boundaries check (see comment).

Have you looked at my similar attempt here #5 ?
We're pretty much doing the same thing, and I think we have the same issues that are listed in #5


if (!grammar) {
return text.characters;
}

const tokens = Prism.tokenize(string, grammar);
let offset = 0;
if (opts.blockPerLine && block === previousBlock) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Caching the last computed tokens is a nice optimization. Improves the first decoration pass.
I'll add it to my PR

@@ -44,53 +44,113 @@ function PrismPlugin(opts) {
opts.onlyIn = opts.onlyIn || defaultOnlyIn;
opts.getSyntax = opts.getSyntax || defaultGetSyntax;
opts.renderToken = opts.renderToken || defaultTokenRender;
opts.blockPerLine = opts.blockPerLine || false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking to make it the default behavior (and not have an option for it)


const gap = Math.max(start - offset, 0)
let s = offset - start + gap;
let e = nextOffset - start + gap;
Copy link
Contributor

@Soreine Soreine Oct 11, 2017

Choose a reason for hiding this comment

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

I think there's a bug here when nextOffset > end

Testing with this text:

<!-- Some 
HTML -->

We have a call addMark(0, 18, 'comment') on the first text line, that fails because the comment mark extends beyond the end of the text.

@Soreine
Copy link
Contributor

Soreine commented Oct 11, 2017

I'm glad that you took a look at it. I haven't worked on it since then. I think the only blocker is ianstormtaylor/slate#893

@ianstormtaylor
Copy link
Author

@Soreine oops! I totally didn't see or remember that you already had worked on this in #5 😄 I like your approach!

I'll check out that Slate issue today or tomorrow hopefully!

@Soreine
Copy link
Contributor

Soreine commented Oct 16, 2017

Closed in favor of #5

@Soreine Soreine closed this Oct 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants