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

Line Highlight: Account for offset when clamping ranges #3518

Merged
merged 1 commit into from Aug 23, 2022

Conversation

RunDevelopment
Copy link
Member

This fixes #3517.

This was caused by #3475. I failed to take the offset into account when giving my suggestion.

Screenshot of the example in #3517 working.

image

@github-actions
Copy link

JS File Size Changes (gzipped)

A total of 1 files have changed, with a combined diff of +1 B (+0.1%).

file master pull size diff % diff
plugins/line-highlight/prism-line-highlight.min.js 1.52 KB 1.52 KB +1 B +0.1%

Generated by 🚫 dangerJS against 95c2e4d

Copy link
Collaborator

@JaKXz JaKXz left a comment

Choose a reason for hiding this comment

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

I see that the line-highlight plugin does not have any tests. I think adding one would be good as part of this PR, if possible

@RunDevelopment
Copy link
Member Author

I added tests in #3524. This PR is now blocked by #3524.

@RunDevelopment
Copy link
Member Author

Alright, so #3524 is not gonna happen until #3408 which require v2. So either we change our stance on #3408 or we merge this PR without tests and add them in later.

I don't want to have a bunch of long expected HTML in the test files, so I don't want to do these tests without snapshots.

Also, I'll likely have a bit of time on my hands next week, so I could start work with v2. So merging this PR now, making the last v1 release with weekend, and then starting with v2 would be my preferred timeline.

Thoughts? @JaKXz @mAAdhaTTah @Golmote @LeaVerou

@mAAdhaTTah
Copy link
Member

we merge this PR without tests and add them in later.

That seems fine. Let's go w/ it.

Copy link
Collaborator

@JaKXz JaKXz left a comment

Choose a reason for hiding this comment

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

Agreed with @mAAdhaTTah 👍🏽

@RunDevelopment RunDevelopment merged commit 098e300 into PrismJS:master Aug 23, 2022
@RunDevelopment RunDevelopment deleted the issue3517 branch August 23, 2022 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some lines are not highlighted
3 participants