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 highlightjs usage to prevent report from lagging/crashing #527 #533

Merged
merged 4 commits into from Feb 4, 2019

Conversation

yong24s
Copy link
Contributor

@yong24s yong24s commented Feb 2, 2019

Fixes #527

hightlightjs (`hljs`) is used for highlighting of codes in code view
giving it context and readability. It can be triggered to render an
element with `window.hljs.highlightBlock(element)`.

`hljs` operations are expensive and stateless, where it re-renders even
on a rendered DOM and uses automatic detection of programming languages
if the language is not specified. Also, multiple `hljs` operations can
be triggered at once, causing the report to lag and crash.
Additionally, it seems to render regardless of trigger for any code in
`<pre><code>`. Furthermore, in our frontend, `vuejs` will trigger `hljs`
in `Vue#update`, when there is a data change, causing DOM to be 
re-rendered.

Let's optimize hljs usage, by triggering it when code is inserted into
the DOM, enclosing the code with a different tag, specifiying the
language to prevent expensive auto detection.

@yong24s
Copy link
Contributor Author

yong24s commented Feb 2, 2019

@ongspxm

Can help me with the ESLint errors? 😢

I got it fixed. Just check if my fixes are correct. 😀

@yong24s yong24s requested a review from ongspxm February 3, 2019 05:02
@yong24s
Copy link
Contributor Author

yong24s commented Feb 3, 2019

@eugenepeh

For your preliminary testing, please.

@yong24s yong24s changed the title highlightjs is triggered multiple times causing report to lag/crash #527 Improve highlightjs usage to prevent report from lagging/crashing #527 Feb 3, 2019
Copy link
Member

@eugenepeh eugenepeh left a comment

Choose a reason for hiding this comment

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

Excellent! 💯 👍

Copy link
Contributor

@yamidark yamidark left a comment

Choose a reason for hiding this comment

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

Nice, much faster now 💯 👍 🥇

Copy link
Contributor

@ongspxm ongspxm left a comment

Choose a reason for hiding this comment

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

Excellante 👍

const element = ele;
const fileExtension = binding.value.split('.').pop();

element.className = fileExtension;
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit, just set element.className to binding.value.split dun need make new var

@eugenepeh eugenepeh merged commit cbccfbc into reposense:master Feb 4, 2019
@emer7
Copy link
Contributor

emer7 commented Feb 5, 2019

I think HighlightJS doesn't highlight the syntax for subsequent contributor now:

Let's say there are 2 contributor A and B. Upon opening's A code panel it highlights the syntax but it doesn't highlight B's code panel.

Is it caused by this PR?

@eugenepeh
Copy link
Member

@emer7 I am unable to replicate this. Would need you to provide more details. Note that adoc doesn't have much syntax highlighted as the other file types. If you suspect that this is caused by the PR, do use netlify to verify. To do so, visit the deploy url for this commit n the one before e.g. https://deploy-preview-533--reposense.netlify.com/, replace the 533 with target PR number.

@emer7
Copy link
Contributor

emer7 commented Feb 6, 2019

Found it, turns out that problem itself is fixed by this PR. Sorry for the false alarm

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

Successfully merging this pull request may close these issues.

None yet

5 participants