-
Notifications
You must be signed in to change notification settings - Fork 55
fix(docs): EDGE could not open the docs #571
Conversation
We had a similar issue in SUIR repo, Semantic-Org/Semantic-UI-React#3163. I enforced It there any reason why we need |
Thanks for the suggestion @layershifter All script/styles resources urls are now https, and I confirmed that it works on Edge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please, ensure that CHANGELOG.md
entry will be present before merging. Thank you!
<script | ||
crossOrigin="true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The crossorigin
attribute let's us see errors that are thrown in these scripts. Without it, the error will not be logged in the console and debugging is impossible.
When copied from Semantic UI React's Document.js
I failed to change the camel cased crossOrigin
to crossorigin
. If this is fixed, is Edge happy? We'd like to have these properties still.
@@ -26,27 +26,24 @@ | |||
</script> | |||
</head> | |||
<body> | |||
<script src="//cdnjs.cloudflare.com/ajax/libs/anchor-js/3.2.1/anchor.min.js"></script> | |||
<script src="https://cdnjs.cloudflare.com/ajax/libs/anchor-js/3.2.1/anchor.min.js"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did I normalize all scripts to //
?
Scripts without the protocol (http / https) allow the browser to request the script using the current protocol. In dev mode, http, this would allow scripts to be fetched over http which is much faster than https. This is valid HTML per the specifications as you can see here.
Which should we use?
In fact, it seems I needed to update my best practices. You can see more from Paul Irish here. The short answer is https is encouraged for all sources, it is no longer a perf concern, and protocol-less urls are considered an anti-pattern.
Your changes here are correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's first try fixing the crossorigin
casing and testing in Edge before merging. We really want to be able to read errors raised in CDN scripts during development.
@levithomason Edge is not working with neither of this four combinations: |
@levithomason - I think the casing with capital O is correct - |
@miroslavstastny that was the only combination I missed yesterday, thanks for pointing that out. Confirmed, it works on edge. Agreed that we should use |
Removed crossOrigin='true' from the new added
script
tags inindex.ejs
, because edge has some issues with those, resulting in - the docs can't be opened in edge. If there is some solid reason why these were added, please let me know. @levithomason @miroslavstastny @kolaps33