ember-learn / guidemaker-ember-template

Guidemaker template for Ember Guides websites
https://guidemaker-ember-template.netlify.app/release
MIT License
5 stars 22 forks source link

Prevent a re-render from duplicating diff ops #56

Closed jbryson3 closed 2 years ago

jbryson3 commented 4 years ago

While investigating https://github.com/ember-learn/guides-source/issues/1548 I noticed that we are wrapping the diffs in a couple of DOM elements.

if (operator === '+') {
  lines[lineNo - 1] = `<span class="diff-insertion"><span class="diff-operator">+</span>${text}</span>`;
} else {
  lines[lineNo - 1] = `<span class="diff-deletion"><span class="diff-operator">-</span>${text}</span>`;
}

This is fine when it is first rendered. However, if the didRender event fires again, the Prism.highlightAll(); fires again, but this time with the wrapped code. Since the wrapped code includes a DOM element with innerText (.diff-operator), this diff operator is included, and then we wrap it again, resulting in multiple ops for diffs.

I removed the <span class="diff-operator">*</span> since it was a user hidden element, and didn't seem to be used anywhere else.

mansona commented 4 years ago

so... we haven't had a chance to do this yet, but the implementation that is in the guidemaker-ember-template will likely be replaced with https://github.com/empress/ember-showdown-prism very soon. I don't know if that implementation will fix this specific problem, but it fixes the other issue we have were the highlighted code isn't available if you don't have JS enabled (and that's what's causing the "flash of unstyled code")

There is a chance that we can just replace it now but I have been waiting until I start looking at this app as part of the Ember Website Redesign project, sorry for the delay 🙃

@jbryson3 do you want to try replacing the implementation with ember-showdown-prism and see what happens?

mansona commented 2 years ago

When I said that this implementation will likely be replaced with ember-showdown-prism that is now a certainty. The new redesign branch is using that addon and if you think that we have the same issue we can make the change over there 👍 but the fact that it works properly on the server-side-rendering version means that I'm pretty sure we won't have the scroll issue any more 🎉