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

[Site] Remove highlight.php #1235

Merged
merged 1 commit into from
Dec 19, 2023

Conversation

smnandre
Copy link
Member

Q A
Bug fix? yes
New feature? yes
Issues Fix #683
License MIT

Remove highlight.php and use highlight.js directly.

(highlight.php is in fact abandonned)

@smnandre
Copy link
Member Author

Just a first step to remove the PHP package easily... i keep in mind the @kbond suggestion to use shiki

@weaverryan
Copy link
Member

The issue I have with shiki is that it's a Node package that you run on the server. And I think you'd even need Node on production (not just locally) so it can be called at runtime. I'm not real excited by that.

@smnandre
Copy link
Member Author

smnandre commented Nov 6, 2023

The issue I have with shiki is that it's a Node package that you run on the server. And I think you'd even need Node on production (not just locally) so it can be called at runtime. I'm not real excited by that.

I agree and still looking for a more integrated solution. In the meanwhile i think the JS version is OK and provide good results.

@smnandre smnandre force-pushed the site/remove-highlight-php branch from 9f7c09d to 307ee2f Compare November 8, 2023 20:20
@smnandre
Copy link
Member Author

smnandre commented Nov 8, 2023

Rebased and ready

@wouterj
Copy link
Member

wouterj commented Nov 8, 2023

Mostly for my own interest (as we're using highlight.php in the Symfony docs as well, and I'm working integrating it in our new rst-parser): is there something truly wrong about highlight.php? We can override the PHP grammar with a customized grammar (we do this in the Symfony docs), and we can even try loading the latest highlight.js grammar. Or is there a BC break in the new grammars that causes highlight.php to no longer support them?

Btw, I've been side-tracked in creating a PHP version of shiki - i.e. using TextMate grammars (used by VScode and Jetbrains) to highlight code using a PHP library. But I'm facing some challenges with supporting TextMate grammars. Maybe you'll see a new highlighter some day in my GitHub 😉

@smnandre
Copy link
Member Author

smnandre commented Nov 8, 2023

Mostly for my own interest (as we're using highlight.php in the Symfony docs as well, and I'm working integrating it in our new rst-parser): is there something truly wrong about highlight.php? We can override the PHP grammar with a customized grammar (we do this in the Symfony docs), and we can even try loading the latest highlight.js grammar. Or is there a BC break in the new grammars that causes highlight.php to no longer support them?

For what it's worth, I'm not too fond of using abandoned packages / librairies... and it seems this is the case for this one (as the author explained multiple times he's working in another direction)

I don't know if those are "BC" or just easy fix, but currently on the ux.symfony.com pages (using hightlight php) the PHP attributes cannot be selected, for example.

Also, i'd really want to demo some things with CSS 4 stuff in the next weeks/months, and i'd like to avoid spending too much time checking everytime if this is working in highlight.php or if i need to find some time to fix it. But maybe i'm too pessimist and everything is ok.

So i genuinely don't know if we can easily "fix what need to be fixed" or if this is deeper than that. And i'd love to ear more about your experience with it, and your view on what/how we should use in the next monthes!

@smnandre
Copy link
Member Author

smnandre commented Nov 8, 2023

Btw, I've been side-tracked in creating a PHP version of shiki - i.e. using TextMate grammars (used by VScode and Jetbrains) to highlight code using a PHP library. But I'm facing some challenges with supporting TextMate grammars. Maybe you'll see a new highlighter some day in my GitHub 😉

Shiki was the advice @kbond gave me at first , but a full PHP version would be even better!

I'd love to beta-test your solution when/if you want to share it !

@smnandre
Copy link
Member Author

Do you see something blocking here @weaverryan ?

ux.symfony.com/templates/components/CodeBlock.html.twig Outdated Show resolved Hide resolved
@@ -96,15 +93,17 @@ public function getGithubLink(): string
return sprintf('https://github.com/symfony/ux/blob/2.x/ux.symfony.com/%s', $this->filename);
}

private function getLanguage(): string
public function getLanguage(): string
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be public - I couldn't spot what is using it

Copy link
Member Author

Choose a reason for hiding this comment

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

Time to understand again... in the template (and that was not done, thank you)

Before we gave this value to the Highlight PHP lib... now we need to pass it to the twig template and set a "language-XXX" class.

@smnandre smnandre force-pushed the site/remove-highlight-php branch from b1752c9 to 1f77cd4 Compare December 18, 2023 23:15
@smnandre smnandre requested a review from weaverryan December 19, 2023 00:03
@weaverryan weaverryan force-pushed the site/remove-highlight-php branch from 681bd27 to 572d719 Compare December 19, 2023 11:22
@weaverryan
Copy link
Member

Thanks Simon!

@weaverryan weaverryan merged commit 857166e into symfony:2.x Dec 19, 2023
1 check passed
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.

[Site] Change to use highlight.js directly
4 participants