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

Adding back code-highlight to blog post, tweaked performance issue with scrolling. #569

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

Conversation

juancabrera
Copy link

This PR covers:

  1. Adding back code highlighting to blog posts, fixed issue with multiple nesting and multiple re-rendering.
  2. Added throttle of 240ms to main onScroll event listener to help with performance.

Notes:

  • Code highlighting is working but it's trying to detect what language is for each block, as right now there is no way to pass the language. This eventually will need to get implemented for optimal results.

@phil-linnell
Copy link
Contributor

@juancabrera

  1. Great, nice work here. Do we need nextProps, nextState arguments as they are not used?
  2. Much improved performance on pages that don't utilise the scroll listener which is nice. However, I'm not enjoying the clunky feeling on areas that DO rely on the scroll listener, e.g. scroll back to the top on home or work page. As a result I feel like we should approach this slightly differently. I need to get some time to look into how we can improve when and where we use the scroll listener, but please jump in with any thoughts.

Finally, the language support: I guess, @andydutch, this could be a feature to implement - some way to choose the language for each block in WP. At the moment the automatic detection is useless and I don't think we can justify the introduction of syntax highlighting unless we solve it. What do you think?

@juancabrera
Copy link
Author

@phil-linnell agree with you. Do you think that lower down the throttle to maybe 100ms will help ?.

@andydutch let us know what you think! and how we can help to make this happen.

@phil-linnell phil-linnell force-pushed the feat/code-hightlight-performance branch from 066004f to a7bf5c5 Compare August 9, 2017 11:23
@phil-linnell
Copy link
Contributor

@juancabrera Changing the throttle to 100ms didn't help. Based on my thoughts about performance elsewhere in the app, I thought maybe just to return to no throttling, and instead remove the listener on post and caseStudy pages. What do you think?

@juancabrera
Copy link
Author

@phil-linnell gotcha.

about the listener, when I chatted with casey and fredrik one of the most useful things of the listener was to know if people finished (or how far read) the blog post. So based on that it should be on the post page. In fact, I tried removing it and works like a sharm.

they were open to change or other options, so let me check with them again. Other way to get a sense of how much the people reads the blogpost is by how much time they spent on the post.

will get back to you asap.

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.

2 participants