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

[RFC][Site] replace highlight.js with tempest/highlight #1742

Closed
wants to merge 3 commits into from

Conversation

kbond
Copy link
Member

@kbond kbond commented Apr 16, 2024

Q A
Bug fix? no
New feature? no
Issues n/a
License MIT

(depends on #1740)

I couldn't remove highlight.js entirely as translation-demo-controller.js still requires it and I don't think we can easily refactor it out. I don't really want to maintain two syntax highlighters (+styling) so I'm not sure this PR is worth it...

@carsonbot carsonbot added the Status: Needs Review Needs to be reviewed label Apr 16, 2024
@@ -13,7 +13,7 @@
<pre
{% if height %} style="--height: {{ height }}" {% endif %}
data-clipboarder-target="source"
><code {% if highlight %} data-code-highlighter-target="codeBlock" {% endif %}>{{ __code ? __code|trim|raw : code }}</code></pre>
><code>{{ __code ? __code|trim|raw : code }}</code></pre>
Copy link
Member Author

Choose a reason for hiding this comment

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

I broke the highlighting here so this needs to be fixed. Does highlight.js detect the language?

@@ -21,7 +21,7 @@ jobs:

- uses: shivammathur/setup-php@v2
with:
php-version: 8.1
php-version: 8.3
Copy link
Member

Choose a reason for hiding this comment

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

Hahaha ... hidden in a PR :))

@smnandre
Copy link
Member

I don't think we can easily refactor it out

It's not that hard i think. Let's add this to my ever-growing-todo-list 😅

@Kocal
Copy link
Member

Kocal commented Apr 17, 2024

That's a nice improvement, especially for web perfs 👀

For translator-demo-controller.js, maybe for a next iteration, we can try to highlight code blocks with tempest/highlight and update them in front-side. Or maybe use LiveComponents, but it can be overkill

Copy link
Collaborator

@WebMamba WebMamba left a comment

Choose a reason for hiding this comment

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

Humm sorry but I think if do this move we should completly remove highligh.js even in translator-demo-controller 😅

@kbond
Copy link
Member Author

kbond commented Apr 18, 2024

Humm sorry but I think if do this move we should completly remove highligh.js even in translator-demo-controller

I agree, closing.

@kbond kbond closed this Apr 18, 2024
use Tempest\Highlight\Highlighter;

/**
* TODO: remove this class when/if https://github.com/tempestphp/highlight/pull/102 is merged.
Copy link
Member

Choose a reason for hiding this comment

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

kbond added a commit that referenced this pull request Apr 30, 2024
…(kbond)

This PR was merged into the 2.x branch.

Discussion
----------

[Site] switch from `highlight.js` to `tempest/highlight`

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| Issues        | n/a
| License       | MIT

Continuation of #1742. I now fully removed highlight.js.

Commits
-------

22ef22e [Site] switch from `highlight.js` to `tempest/highlight`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Needs Review Needs to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants