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

Update prism.js to match new webhelp highlight colors #2670

Merged
merged 5 commits into from
Sep 20, 2022

Conversation

IgnatBeresnev
Copy link
Member

No description provided.

@@ -792,7 +800,6 @@ open class HtmlRenderer(
TextStyle.Strong -> strong { body() }
TextStyle.Var -> htmlVar { body() }
TextStyle.Underlined -> underline { body() }
is TokenStyle -> span("token " + styleToApply.toString().toLowerCase()) { body() }
Copy link
Member Author

Choose a reason for hiding this comment

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

Previously, if a text element had >2 TokenStyle styles, it was rendered as

span("token first") {
    span("token second") {
        text
    }
}

@IgnatBeresnev
Copy link
Member Author

IgnatBeresnev commented Sep 19, 2022

JFYI: For some reason the new version of prism.js (1.29.0) parses inline comments incorrectly and includes everything post-comment under the comment token.

Unable to reproduce it on their playground, so maybe it's a bug of Dokka (incorrect line breaks or something) - needs to be investigated furtner. It can be postponed, so I downgraded it to the version that we had before that.

2022-09-19_17-35-34

@IgnatBeresnev IgnatBeresnev marked this pull request as ready for review September 19, 2022 15:41
@vmishenev
Copy link
Member

JFYI: For some reason the new version of prism.js (1.29.0) parses inline comments incorrectly and includes everything post-comment under the comment token.

I suppose you have not copied this hook.

@@ -71,9 +71,10 @@ class HighlightingTest : BaseAbstractTest() {
assertTrue(children?.get(it.first)?.style?.contains(it.second) == true)
val annotation = children?.first()?.children?.first()

assertTrue(annotation?.children?.get(0)?.style?.contains(TokenStyle.Annotation) == true)
assertTrue(annotation?.children?.get(1)?.children?.first()?.style?.contains(TokenStyle.Annotation) == true)
val annotationStyles = setOf(TokenStyle.Annotation, TokenStyle.Builtin)
Copy link
Member

Choose a reason for hiding this comment

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

What does TokenStyle.Builtin mean?
I am not sure the output-specific problem should be fixed here, in the content model rather than in the render stage.

Copy link
Member Author

Choose a reason for hiding this comment

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

What does TokenStyle.Builtin mean?

It's a good question, you were the one that added the type :) It was already in Public API, so I just used it.

I am not sure the output-specific problem should be fixed here, in the content model rather than in the render stage.

Sure, I can fix it for html-only, but let's then delete TokenStyle.Builtin entry so there's no confusion?

Copy link
Member

Choose a reason for hiding this comment

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

It's a good question, you were the one that added the type :) It was already in Public API, so I just used it.

I hoped somebody tells me what it means)
You can remove it since it is not used anywhere.

@IgnatBeresnev
Copy link
Member Author

  • Upgraded prism.js, added the hook and left a comment for future generations
  • Removed TokenStyle.Builtin entry, now adding builtin class only for html pages
  • Reverted some of the unnecessary changes (the ones before review)

@IgnatBeresnev IgnatBeresnev merged commit c63ec45 into master Sep 20, 2022
@IgnatBeresnev IgnatBeresnev deleted the prismjs-webhelp-styles branch September 20, 2022 12:42
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.

None yet

2 participants