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

[highlightjs] Add new addon #737

Merged
merged 2 commits into from Oct 1, 2018

Conversation

Projects
None yet
3 participants
@MrPetovan
Collaborator

MrPetovan commented Sep 16, 2018

Related to friendica/friendica#5765

Since we aren't doing syntax highlighting in the core, I'd figured people would like a replacement, and this is the one.

Depends on friendica/friendica#5773

@annando

This comment has been minimized.

Collaborator

annando commented Sep 17, 2018

We are so late in the release that I really have problems in merging some huge PR like this and the corresponding one in the core. We surely would need several days of testing - but I would prefer having the release this week.

@MrPetovan

This comment has been minimized.

Collaborator

MrPetovan commented Sep 17, 2018

This can actually wait, the other can't. No post with highlighted code was coming through at all.

@MrPetovan

This comment has been minimized.

Collaborator

MrPetovan commented Sep 17, 2018

The only interesting file in this addon is highlightjs.php, the rest is the JS library itself.

@rabuzarus

This comment has been minimized.

Contributor

rabuzarus commented Sep 18, 2018

Does it work with new content added by ajax (next page / new posts) ?

@MrPetovan

This comment has been minimized.

Collaborator

MrPetovan commented Sep 18, 2018

That's an excellent question, I haven't been able to see this in the wild yet, but I don't believe it would.

@MrPetovan

This comment has been minimized.

Collaborator

MrPetovan commented Sep 19, 2018

Issue confirmed, no syntax highlighting on ajax (re)load.

@rabuzarus

This comment has been minimized.

Contributor

rabuzarus commented Sep 19, 2018

For new posts:
I would say we would need a new js hook in main.js before we update the scroll position. Maybe at the end of updateConvItems(). When the hook is called the addon would need to execute something like

$('pre > code').each(function() {
     hljs.highlightBlock(this);
})

(highlightjs/highlight.js#966 (comment))
or

var blocks = document.querySelectorAll('pre code:not(.hljs)');
Array.prototype.forEach.call(blocks, highlightBlock);)

(highlightjs/highlight.js#909 (comment))

Same for loadScrollContent();

or we use another javascript code highlighter

@rabuzarus

This comment has been minimized.

Contributor

rabuzarus commented Sep 19, 2018

or we use another javascript code highlighter

maybe Prism (http://prismjs.com). We would execute Prism.highlightAll(); at the end of updateConvItems() (https://schier.co/blog/2013/01/07/how-to-re-run-prismjs-on-ajax-content.html) or if we want to have an eye on performance we could use Prism.highlightAllUnder(element, async, callback) for the new content (https://prismjs.com/extending.html).

@MrPetovan

This comment has been minimized.

Collaborator

MrPetovan commented Sep 19, 2018

Thank you for looking into it, my plan was to add an custom event on the <body> tag triggered by the Ajax load and then add an event listener in the <script> tag added by the addon. This is the most unobtrusive way of handling this, and it may be used for other addons as well without having to tamper with the main JS routine.

@MrPetovan MrPetovan changed the base branch from 2018.08-rc to develop Sep 19, 2018

@MrPetovan MrPetovan changed the base branch from develop to 2018.08-rc Sep 19, 2018

@MrPetovan MrPetovan added the 2018.12 label Sep 19, 2018

@MrPetovan MrPetovan force-pushed the MrPetovan:task/add-new-highlightjs-addon branch from 1960411 to 064e6ed Sep 20, 2018

@MrPetovan MrPetovan changed the base branch from 2018.08-rc to develop Sep 20, 2018

@MrPetovan MrPetovan force-pushed the MrPetovan:task/add-new-highlightjs-addon branch 2 times, most recently from ae2cd92 to 8056a5a Sep 20, 2018

@MrPetovan MrPetovan force-pushed the MrPetovan:task/add-new-highlightjs-addon branch from 8056a5a to 0afab99 Sep 20, 2018

@MrPetovan

This comment has been minimized.

Collaborator

MrPetovan commented Sep 20, 2018

Phew!

I managed to make syntax highlighting work with Ajax, but not without completely reworking the Javascript hooks and rewriting the MathJax addon in the process 🤔

$b .= <<< HTML
<script type="text/javascript" src="{$basedir}/highlight.pack.js"></script>
<script type="text/javascript">

This comment has been minimized.

@rabuzarus

rabuzarus Sep 20, 2018

Contributor

I would prefer to put the following javascript code into an own js file. We should avoid to mix php and js if it is possible

This comment has been minimized.

@MrPetovan

MrPetovan Sep 20, 2018

Collaborator

Performance-wise, it isn't ideal to have a myriad of tiny files to load, what are you afraid of?

This comment has been minimized.

@rabuzarus

rabuzarus Sep 20, 2018

Contributor

separate js files are good for:

  • debugging (js console in browser - which isn't possible with js code inside html)
  • readability and recognizability of js code (in browser developer console or when looking into the files -> at the moment you need to read the whole html code in the browser to know which js code is executed)
  • clean code (php files should have php code, js files javascript and template files html - I know we have many other cases where we have code of other languages inside of php files but we should work to avoid this if we can)

according to the performance:

  • I thought the js files were cached in the browser

This comment has been minimized.

@MrPetovan

MrPetovan Sep 20, 2018

Collaborator

They are cached, but the first load is more expensive when it doesn't necessarily has to be.

I'll see what I can do.

Hypolite Petovan
[highlightjs] Use new 'head'/'footer' hooks
- Create separate addon javascript file
- Register stylesheet and Javascript files
@MrPetovan

This comment has been minimized.

Collaborator

MrPetovan commented Sep 21, 2018

I added ways to register stylesheets and javascript files so that no HTML is written by hand in the hooks.

I had to create two new hooks and remove a completely useless part of App->page, but it's ready now.

@MrPetovan

This comment has been minimized.

Collaborator

MrPetovan commented Oct 1, 2018

This is ready to merge now.

@annando annando merged commit 81409f0 into friendica:develop Oct 1, 2018

@MrPetovan MrPetovan deleted the MrPetovan:task/add-new-highlightjs-addon branch Oct 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment