Skip to content
This repository has been archived by the owner on Aug 26, 2022. It is now read-only.

Fix issue #283, add dns-prefetch, preconnect for editor iframe #4455

Merged
merged 1 commit into from
Oct 19, 2017
Merged

Fix issue #283, add dns-prefetch, preconnect for editor iframe #4455

merged 1 commit into from
Oct 19, 2017

Conversation

schalkneethling
Copy link

Adds dns-prefetch and preconnect for the editor iframe to test the performance impact.

@codecov-io
Copy link

codecov-io commented Oct 6, 2017

Codecov Report

Merging #4455 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4455   +/-   ##
=======================================
  Coverage   94.71%   94.71%           
=======================================
  Files         260      260           
  Lines       22805    22805           
  Branches     1674     1674           
=======================================
  Hits        21600    21600           
  Misses        981      981           
  Partials      224      224

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a608faa...d5f55ca. Read the comment docs.

{% block extrahead %}
<link rel="dns-prefetch" href="https://interactive-examples.mdn.mozilla.net" pr="0.75" />
<link rel="preconnect" href="https://interactive-examples.mdn.mozilla.net" pr="0.75" />
{% endblock %}
Copy link
Author

Choose a reason for hiding this comment

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

I felt confident that this is the correct file but, now I am not so sure. When I look at a page such as http://127.0.0.1:8000/en-US/docs/Experiment:InteractiveEditor/Array.prototype.reduce() I do not see the link elements in the header :(

Copy link
Contributor

Choose a reason for hiding this comment

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

That's because it is overridden in document.html, which is a better place for this code:

https://github.com/mozilla/kuma/blob/a608faaa7c190b094fe15eda33877050e34ecb95/kuma/wiki/jinja2/wiki/document.html#L66-L86

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @jwhitlock Will update the PR

@stephaniehobson
Copy link
Contributor

Do we think there are any downsides to adding it to all pages?

I think it is possible to include the code only if the interactive examples are on the page. We do some custom configuration on the kuma side in response to the existence of the quick-links class for left side menus. (I'm not sure enough about how it works to give instructions though).

@schalkneethling
Copy link
Author

I think it is possible to include the code only if the interactive examples are on the page.

@stephaniehobson That would be great, as long as it is not injected with JS. I cannot find the doc now, but I remember recently hearing something about Chrome not behaving predictably if you inject these tags with JS. I will try and find the reference to it. Perhaps it is under x condition which does not apply here.

@stephaniehobson
Copy link
Contributor

Yes! That's it exactly. Great detective work :)

Copy link
Contributor

@stephaniehobson stephaniehobson left a comment

Choose a reason for hiding this comment

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

Good now that John's changes are integrated. My comments can be acted on if we see performance problems.

@stephaniehobson stephaniehobson merged commit f34f7da into mdn:master Oct 19, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants