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 Prism JS for syntax highlighting #32

Merged
merged 2 commits into from
Sep 25, 2023
Merged

Add Prism JS for syntax highlighting #32

merged 2 commits into from
Sep 25, 2023

Conversation

malberts
Copy link
Contributor

@malberts malberts commented Sep 24, 2023

Refs #30

  • Drops PHP 7.4 support (because it's too annoying to write new code like that, unless there's a hard requirement to keep it)
  • Added additional parser function parameters based on SyntaxHighlight:
  • If lang is specified then the code syntax highlighter is used
    • Default behavior is still Markdown
    • This allows Markdown to be rendered as code too
  • Includes a copy plugin which shows a button when hovering over the code

Follow-up TODOs:

  • Update syntax highlight CSS to match SyntaxHighlight extension (as close as possible, might not be 100% due to differen tokenization)

Nice to have TODOs:

  • Auto-detect the language based on file extension (if lang is not specified)
  • Improve how Prism JS/CSS is included. Currently it is this bundle downloaded from their website.
  • Make copy plugin optional? Probably an additional copy parameter like line.
  • Allow using other Prism plugins?

@malberts malberts force-pushed the prismjs branch 2 times, most recently from 73887b0 to 8f9394c Compare September 24, 2023 20:02
@malberts
Copy link
Contributor Author

This wikitext

==Markdown==
===No args (=Markdown)===
{{#bitbucket:http://bitbucket:7990/projects/TEST/repos/test/browse/README.md}}
===lang===
{{#bitbucket:http://bitbucket:7990/projects/TEST/repos/test/browse/README.md|lang=markdown}}
===lang,line===
{{#bitbucket:http://bitbucket:7990/projects/TEST/repos/test/browse/README.md|lang=markdown|line}}

==Python==
===No args (=Markdown)===
{{#bitbucket:http://bitbucket:7990/projects/TEST/repos/test/browse/test.py}}
===lang===
{{#bitbucket:http://bitbucket:7990/projects/TEST/repos/test/browse/test.py|lang =python}}
===lang,line===
{{#bitbucket:http://bitbucket:7990/projects/TEST/repos/test/browse/test.py|lang=python|line}}

==Typescript==
===No args (=Markdown)===
{{#bitbucket:http://bitbucket:7990/projects/TEST/repos/test/browse/test.ts}}
===lang===
{{#bitbucket:http://bitbucket:7990/projects/TEST/repos/test/browse/test.ts|lang= typescript}}
===lang,line===
{{#bitbucket:http://bitbucket:7990/projects/TEST/repos/test/browse/test.ts|lang=typescript|line}}

==HTML==
===No args (=Markdown)===
{{#bitbucket:http://bitbucket:7990/projects/TEST/repos/test/browse/test.html}}
===lang===
{{#bitbucket:http://bitbucket:7990/projects/TEST/repos/test/browse/test.html|lang=html}}
===lang,line===
{{#bitbucket:http://bitbucket:7990/projects/TEST/repos/test/browse/test.html|lang = html|line}}
===Broken===
{{#bitbucket:http://bitbucket:7990/projects/TEST/repos/test/browse/testhtml|lang=html}}

==No language==
{{#bitbucket:http://bitbucket:7990/projects/TEST/repos/test/browse/test.py|lang=    |line}}
{{#bitbucket:http://bitbucket:7990/projects/TEST/repos/test/browse/test.ts|lang|line=}}
{{#bitbucket:http://bitbucket:7990/projects/TEST/repos/test/browse/test.html|lang=|line=foo}}

renders this:
image

@JeroenDeDauw
Copy link
Member

So {{#embed:foobar.php}} will still render the PHP as markdown? (Not a regression so not a blocker)

Drops PHP 7.4 support (because it's too annoying to write new code like that, unless there's a hard requirement to keep it)

Yes, nice to drop 7.4

line: whether to show line numbers

Is that the same parameter name as used by the MW extension? line makes me think about what lines should be shown or highlighted. Something like showLineNumbers would be more clear, though harder to remember.

I'm also a bit concerned about adding code specific parameters lang and line to a generic embed function. But I don't see how to improve on the situation.

@malberts
Copy link
Contributor Author

So {{#embed:foobar.php}} will still render the PHP as markdown? (Not a regression so not a blocker)

Yes. That's basically this TODO in the code: https://github.com/ProfessionalWiki/ExternalContent/pull/32/files#diff-4e52f3cf52e1f852c9ae649d8efeef9f7c7953d4ef619a21b4c1bdb93b2d0d15R30

It's simple enough to just check if the extension is not .md, so if it makes sense for the default behavior then it's quick to do.

Is that the same parameter name as used by the MW extension? line makes me think about what lines should be shown or highlighted. Something like showLineNumbers would be more clear, though harder to remember.

Yes: https://www.mediawiki.org/wiki/Extension:SyntaxHighlight#line. That extension uses highlight for specifying lines to be highlighted. I have no particular preference for the SyntaxHighlight names, I just used that for similarity.

I'm also a bit concerned about adding code specific parameters lang and line to a generic embed function. But I don't see how to improve on the situation.

I couldn't come up with a better approach. Unless we split the embed and bitbucket functions into a further embed-code and bitbucket-code, or something along those lines.

Copy link
Member

@JeroenDeDauw JeroenDeDauw left a comment

Choose a reason for hiding this comment

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

I suspect that it'd be good to change ContentRendererFactory to something like DelegatingContentRenderer. The extractOptions would go to EmbedFunction and BitBucketFuncrtion.

To properly dispatch on file extension, the URL normalization must happen first. Right now the ContentRendererFactory code is executed before the URL is normalized.

Epistemic status: 3 awake braincells

@JeroenDeDauw
Copy link
Member

It's simple enough to just check if the extension is not .md, so if it makes sense for the default behavior then it's quick to do.

Getting behavior that is consistent, convenient, and reasonably in line with existing 1.x behavior seems challenging. Too difficult without sleep first.

@malberts
Copy link
Contributor Author

I'll do a follow-up with code improvements.

@malberts malberts merged commit 496ba39 into master Sep 25, 2023
8 checks passed
@malberts malberts deleted the prismjs branch September 25, 2023 13:34
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.

2 participants