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

Add support for inline code highlighting #3180

Merged

Conversation

JamesTheAwesomeDude
Copy link
Contributor

@JamesTheAwesomeDude JamesTheAwesomeDude commented May 7, 2021

Resolves #945

Changes

This pull request adds an optional parameter to highlightAll(), which can make it use a query selector other than pre code. It does not change any existing behavior of the software.

Checklist

  • Not a grammar change, markup tests not needed.
  • I could not identify which section of the changelog to file this under, either:

This wasn't a deprecation or removal; I'm unclear if it's big enough to bother listing as an API change high-level bullet; it doesn't belong under security, themes, language grammars, parser, grammars, new languages, theme improvements, new themes, or dev improvements (that last one looks like it's to do with changes to your in-house tooling system.)

It's a tiny, optional, backwards-compatible, mostly-user-facing enhancement. But it looks like that's not currently a header/category in the changelog. Should I create it?

Resolves highlightjs#945 for non-technical users such as those using
highlight.js via a WordPress plugin, allowing trivial and much-
less-invasive inclusion of other elements (such as <code>s not
wrapped in a <pre>) than existing proposed solutions
@joshgoebel
Copy link
Member

joshgoebel commented May 7, 2021

This type of item would fall under parser, that's the catch all for overall improvements to the parser, it's API, config, etc... the crazy long release notes now are only because we're in the middle of the 10 -> 11 transition.


While selector does seem a natural choice [as an argument] one could also suggest that other items pertaining directly to how we perform "highlight all" (such as noHighlightRe, classPrefix) might be useful to pass as well, but that is not desirable. We'd prefer to keep this API clean and simple. Configuration should happen via our existing configure API, ie:

hljs.configure({cssSelector: "code"});
hljs.highlightAll();

Are you willing to re-work your PR to instead add a cssSelector configuration option?

@joshgoebel
Copy link
Member

joshgoebel commented May 7, 2021

It's then also worth discussing if our default CSS may need to be changed to offer both the a pre code and code variant of the top-level .hljs selector.

This lives in makestuff.js now.

@JamesTheAwesomeDude
Copy link
Contributor Author

It's then also worth discussing if our default CSS may need to be changed to offer both the a pre code and code variant of the top-level .hljs selector.

Yes, I think that may also require a lot of refactoring.

I'm currently jury-rigging it with a :not selector, but this just screams "FIXME":

code.hljs:not(pre>code) {
  padding: initial;
  display: initial;
}

(This may be idiopathic to WordPress, which has a sort of "convention" — and by "sort of", I mean it's nearly immutably how the HTML is operated while using the software — that Block <code> elements are always designated by being wrapped in <pre>, and that their Inline counterparts are designated by nothing more than not being so wrapped.)


Configuration should happen via our existing configure API, ie:

hljs.configure({cssSelector: "code"});
hljs.highlightAll();

Are you willing to re-work your PR to instead add a cssSelector configuration option?

Certainly! Since I'm currently unfamiliar with your API, you've done 90% of the work for me just there by defining the implementation! 😁 (I've just pushed the changes; how do they look?)

src/highlight.js Outdated Show resolved Hide resolved
docs/api.rst Outdated Show resolved Hide resolved
@joshgoebel
Copy link
Member

joshgoebel commented May 7, 2021

that Block <code> elements are always designated by being wrapped in <pre>, and that their Inline counterparts are designated by nothing more than not being so wrapped.)

That sounds 100% semantically correct to me. So why can't that just be accomplished with two rules?

pre code.hljs {
  display: block;
  overflow-x: auto;
  padding: 0.5em;
}

code.hljs { 
  padding: ...;
}

The block level items then only apply to pre/code as they should... and we only add a small amount of padding to inline elements.

Strongly consider that since it seems that `code:not(pre code)`
are excluded from in the definition of "code blocks"

- therefore, reconsider this?:
https://github.com/highlightjs/highlight.js/blob/11.0.0-beta0/src/styles/default.css#L11-L12
src/highlight.js Outdated Show resolved Hide resolved
Co-authored-by: Josh Goebel <me@joshgoebel.com>
@joshgoebel
Copy link
Member

to remove display: block; from the bare .hljs rule

I don't think we need a separate PR for that, the scope of this one is small enough to finish this work I think.

This will merit a mention in breaking changes now since it would break anyone using div.hljs or something weird (they'll need to update their CSS)... but I can handle that if you don't know where to put it.

@JamesTheAwesomeDude
Copy link
Contributor Author

Yeah, I could use a hand with the documentation / I'm not qualified to tear down chesterton's fence in this case; thanks.

docs/api.rst Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
JamesTheAwesomeDude and others added 2 commits May 7, 2021 05:18
Co-authored-by: Josh Goebel <me@joshgoebel.com>
Co-authored-by: Josh Goebel <me@joshgoebel.com>
CHANGES.md Outdated Show resolved Hide resolved
@JamesTheAwesomeDude JamesTheAwesomeDude changed the title Allow highlightAll to be used with custom selectors Add support for inline code highlighting May 7, 2021
joshgoebel and others added 2 commits May 7, 2021 06:53
Co-authored-by: James <codegeek98+github@gmail.com>
src/highlight.js Outdated Show resolved Hide resolved
@JamesTheAwesomeDude JamesTheAwesomeDude marked this pull request as ready for review May 7, 2021 17:51
@joshgoebel
Copy link
Member

@JamesTheAwesomeDude Thanks so much for working on this.

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.

Highlight.JS Inline Code Syntax Highlightning
2 participants