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

Improve prism.js highlighting #3746

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

solonovamax
Copy link

@solonovamax solonovamax commented Aug 15, 2024

This PR improves the prism.js highlighting in a couple of minor ways.

  • plugin changes:
    • add show-language + toolbar plugins: Shows the language of the current codeblock.
    • add autoloader: Used to automatically load languages.
    • add match-braces: when hovering over a bracket, highlight the matching bracket. Added match-braces class to base.ftl.
    • add diff-highlight: highlight diff codeblocks in conjunction with other languages.
  • remove css and javascript languages from being loaded in the main prism.js file.
  • Add components directory with all additional prism.js supported languages. These are loaded on-demand by the autoloader plugin.
  • Add small transition to the copy code element to match the language toolbar element. (100ms linear transition from 0 -> 100% opacity)
  • Fix minor bug in FileWriter causing additional empty directories to be created when copying a folder with a trailing slash.
  • Make the diff highlighting look better

@whyoleg
Copy link
Collaborator

whyoleg commented Aug 16, 2024

Hey! Could you describe a bit on your use-cases for most of the changes? Specifically for including support for ±300 languages.

@solonovamax
Copy link
Author

solonovamax commented Aug 16, 2024

Hey! Could you describe a bit on your use-cases for most of the changes? Specifically for including support for ±300 languages.

Here's my usecases/reasons for each of the changes:

  • plugin changes:
    • show language: shows the user what language is in use. looks nice.
    • match braces: it looks pretty
    • diff highlight: allows using diff codeblocks paired with language syntax highlighting. also just looks pretty
  • remove css & js language from the main prism.js file: they seemed like less important languages to early-load & they exist in the components directly so they're not removed
  • add support for the 300-odd languages: I ran into a few cases where I wanted a codeblock in some language that wasn't included by default in dokka, so I included those languages in my project via a custom prism.js script. Then I decided that since I'm adding support for a few languages (the specific ones I needed were toml and groovy), might as well add all of them as others might have a use for it. However, I could very much see an argument for wanting to prune it down to the following list & removing all the other ones
    • config formats (xml, toml, json, etc.)
    • scripting languages (bash, batch, zsh, etc.)
    • jvm languages (java, kotlin, groovy, possibly scala)
    • possibly a few other random languages (eg. css & other stuff)
  • transition for copy code button: it looks pretty + it makes it match the language toolbar
  • file writer bug: ran into it while adding code to include the components directory
  • diff highlighting: it looks pretty

not sure how many languages would be left after pruning them, but it'd probably be much less.

@adam-enko
Copy link
Member

Hi, I really like the idea of adding more styling support, but I have some concerns about adding so many files.

Adding these files introduces maintenance and testing requirements on the Dokka team. Dokka has a slow release cycle. so I think it's likely that these scripts would become outdated.

Adding these files could interfere with users who want to use a different highlighting library.
Dokka has support for adding custom scripts for each project, so couldn't adding these be done per-project, rather than as a Dokka default? Although I think Dokka could make this easier, maybe with a Gradle or Dokka plugin?

@solonovamax
Copy link
Author

Hi, I really like the idea of adding more styling support, but I have some concerns about adding so many files.

I'm going to look at pruning a bunch of the niche languages. hopefully that should substantially decrease the file count.

Adding these files introduces maintenance and testing requirements on the Dokka team. Dokka has a slow release cycle. so I think it's likely that these scripts would become outdated.

the last time prism.js was updated was august 23, 2022, so I don't think that's an issue.

Adding these files could interfere with users who want to use a different highlighting library.

how would it interfere with users any more than the current prism.js highlighter in dokka?

Dokka has support for adding custom scripts for each project, so couldn't adding these be done per-project, rather than as a Dokka default? Although I think Dokka could make this easier, maybe with a Gradle or Dokka plugin?

yeah, it could be done per-project, but it's also kinda annoying to have to check in a bunch of js files to vcs if dokka could just have the most common ones.

- plugin changes:
  - add `show-language` + `toolbar` plugins: Shows the language of the current codeblock.
  - add `autoloader`: Used to automatically load languages.
  - add `match-braces`: when hovering over a bracket, highlight the matching bracket.
    Added match-braces class to `base.ftl`.
  - add `diff-highlight`: highlight diff codeblocks in conjunction with other languages.
- remove `css` and `javascript` languages from being loaded in the main prism.js file.
- Add components directory with all additional `prism.js` supported languages.
  These are loaded on-demand by the `autoloader` plugin.
- Add small transition to the copy code element to match the language toolbar element.
  (100ms linear transition from 0 -> 100% opacity)
- Fix minor bug in FileWriter causing additional empty directories to be created
  when copying a folder with a trailing slash.
- Make the diff highlighting look better

Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
@solonovamax solonovamax force-pushed the feature/better-prism-highlighting branch from 43850ee to 4fc4542 Compare September 15, 2024 21:44
@solonovamax
Copy link
Author

solonovamax commented Sep 15, 2024

Alright, I've gone through and pruned some of the less popular languages from 297 languages down to 114 languages.

Are there any other languages I should remove as well? I tried to keep it mostly to popular/useful things that someone might use.

Another alternative could be to change from prism.js altogether and instead use a compile-time syntax highlighter.

I've also rebased on top of master to ensure there were no merge conflicts.

@solonovamax
Copy link
Author

Any update on this?

Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
@solonovamax
Copy link
Author

I've dropped it down to only 91 languages rather than the original 300-odd languages

@solonovamax
Copy link
Author

hey, any updates for this?

@whyoleg
Copy link
Collaborator

whyoleg commented Nov 1, 2024

Hey! Sorry for the delay, we are busy with preparation for Dokka 2.0.0 and beyond :)
I've recently answered in slack

thanks for pinging, but it will be not possible to do it in 2.0 scope
Soon, some HTML changes which are already properly tested will be merged in master, and so it would take some time to test those changes again
But still, I think that some of changes could be a nice addition.
I'm still very sceptic about adding support for additional languages highlighting so I think it would be nice to start without them
Could you update (or create new) PR with removing changes with all those additional languages?
It would also help to speed-up review if you will provide screenshots with before and after with description of what is changed
Thank you!

let's continue discussion here, so that it will be easier to track it

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.

3 participants