Remove jQuery #538
Remove jQuery #538
Conversation
I don't believe we've got a proper list of supported browsers. My perception is that the vast majority of our users are going to be developers or computer-savvy people, and as a very broad rule those sorts of people tend to use a fairly up-to-date version of Chrome or Firefox. If we can support IE then that's awesome, but if it requires large amounts of effort or hacky band-aid solutions to do then I'll leave the decision up to the developer... |
|
Well, ideally we would only load the polyfills for the browsers that need it (feature-detection) - but that is non-trivial without a build system (e.g. webpack). A simpler solution would be to just https://polyfill.io/v2/docs/. What do you think? |
|
I see that crates.io uses Google Analytics. Assuming there is a significant overlap between the audiences, maybe @carols10cents could share some insights about browser usage to guide this decision? |
|
Actually, crates.io does not support IE. I guess we should follow RFC 1985 as well? |
|
I came to this thread to post RFC 1985. |
|
Rebased against master, ready for review. |
src/theme/playpen_editor/editor.js
Outdated
| let editor = ace.edit(this); | ||
|
|
||
|
|
||
| Arom.from(document.querySelectorAll('.editable')).forEach(function(editable) { |
There was a problem hiding this comment.
What is Arom? Is this supposed to be Array?
|
Have you tested this code? Specifically, did you test that the Ace Editor boxes still work? |
|
I mentioned in the description that the changes in |
|
Apparently, I've missed #521 - just tested and it works fine from what I can tell. |
|
Yes, the Ace editor is not yet documented, which is my bad! Some settings need to be enabled for it to work. Generally, I test by running mdBook on the Rust by Example book, since that's what Ace editor was added for originally in the first place, so it's all setup there. I've just tested it with your branch and it seems to be working fine. 😊 |
|
I just checked it out on my machine and I think there's an issue with the code expansion/hiding feature. For example, on this page, here's what it looks like collapsed (the default):
Then hit run and you can see what it prints out:
Expanded:
This is what it's actually meant to look like when expanded:
|
|
@Michael-F-Bryan Thanks, fixed! Sidenote, I really hate how you never know whether you are operating on a single element or on an array in jQuery (in this case, I assumed single element, so only the first hidden line was toggled). |
|
Ah, blast, I didn't check code expansion, good catch @Michael-F-Bryan. |
|
I don't think there were any other noticeable bugs/regression. @sorin-davidoi, do you think this is ready to merge? |
|
Yes, I would say so. |
|
Is jQuery part of the official |
I don't think we've ever said it's guaranteed to be loaded, it just happens to be because we use it internally. I'm going to say that people should make sure they explicitly load jQuery if they need it, plus this PR will be part of a major semver bump, so people shouldn't be surprised if they need to update their |
This is based on #535 for two reasons:I assumed that Improve accessibility #535 is likely to be (eventually) merged, so I can rebase this later againstmasterin that caseMake it easier for reviewers to test both of them at the same timeDepending on which browsers we need to support (couldn't find them listed), we might need the following polyfills:Array.from (no IE)String.trim (no IE 8)Element.closest (no IE)Promise (no IE)fetch (no IE)I couldn't test the changes ineditor.js.Reduces JavaScript payload size from 229.88 KB to 197.91 KB (measured on
book-example). Helps address #475.