Remove jquery dependency #41307

GuillaumeGomez · 2017-04-14T23:23:17Z

r? @rust-lang/docs

frewsxcv · 2017-04-15T00:22:44Z

src/librustdoc/html/static/main.js

+        }
+    }
+
+    function on_each(arr, func) {


Why not just use Array.prototype.forEach?

Simply because it seems that in a lot of cases, this method isn't available (for some reason?). So I wrote this one for all of them and we don't have to check if the method exists or not.

It's not available because many of the methods used, such as getElementsByClassName(), don't return an array, but an array-like object such as HTMLCollection, which indeed doesn't implement forEach(). :/

Is it possible to use Array.from(arr).forEach() here?

@isonmad Really depends on browsers you are targetting, but Array.from is supported only by recent browseres, i.e. not IE. And if that's of no concern, you could also use for/of.

You could use forEach here, although it is generally less performant:

Array.prototype.forEach.call(arr, func)

I made this function exactly because at least every browser supports it. I don't see the point to try to add unneeded things over potential things.

This also doesn't match the naming conventions of the surrounding functions

onEach it is now.

projektir · 2017-04-15T09:48:25Z

Hah, it failed because of what looks like just one tidy error:

[00:02:37] tidy error: /checkout/src/librustdoc/html/static/main.js:580: line longer than 100 chars

I did compare the output (not sure what you mean by navigation as any navigation should also necessarily be part of the output), and everything looks the same besides the commit hash and the removed jquery import, so that looks good.

Looked through the docs manually, as well, and didn't see anything so far.

GuillaumeGomez · 2017-04-15T11:26:42Z

Arf, didn't check tidy. Should be ok now.

jnv · 2017-04-15T17:40:55Z

src/librustdoc/html/static/main.js

@@ -65,18 +113,27 @@
            from = parseInt(match[1], 10);
            to = Math.min(50000, parseInt(match[2] || match[1], 10));
            from = Math.min(from, to);
-            if ($('#' + from).length === 0) {
+            if (document.getElementById(from).length === 0) {


getElementById returns null if the element does not exist, so this will fail.

if (!document.getElementById(from))

Excellent point!

euclio · 2017-04-15T17:52:16Z

src/librustdoc/html/static/main.js

@@ -202,6 +202,8 @@
    document.onkeypress = handleShortcut;
    document.onkeydown = handleShortcut;
    document.onclick = function(ev) {
+        console.log(ev.target);


looks like some console.logs snuck in here

euclio · 2017-04-15T18:29:15Z

I don't see the button to expand/collapse the description anymore. Also, the animations were removed?

GuillaumeGomez · 2017-04-15T20:12:16Z

@euclio: I can add them back, but it'll be through CSS.

jnv · 2017-04-15T20:32:52Z

src/librustdoc/html/static/main.js

@@ -113,7 +113,8 @@
            from = parseInt(match[1], 10);
            to = Math.min(50000, parseInt(match[2] || match[1], 10));
            from = Math.min(from, to);
-            if (document.getElementById(from).length === 0) {
+            var elem = document.getElementById(from);
+            if (!elem || elem.length === 0) {


Thanks for improving this!

However, check for elem.length === 0 is redundant, since getElementById always returns at most a single element since there can be only one element with a given ID. There's no way you could get an array or NodeList of elements from this method. See https://developer.mozilla.org/en-US/docs/Web/API/Document/getElementById

Once again, you're right! I remove it as well.

projektir · 2017-04-15T20:46:25Z

Looked through the code and through the docs. Looks good to me.

johansigfrids · 2017-04-17T08:44:24Z

You don't have to roll your own hasClass, addClass, and removeClass. The classList property on Dom elements has methods for that:

elem.classList.contains('class')
elem.classList.add('class')
elem.classList.remove('class')

GuillaumeGomez · 2017-04-17T10:23:55Z

@johansigfrids: Yes but this isn't supported by old web browsers so I can't use it.

johansigfrids · 2017-04-17T11:13:39Z

How old browsers do you really need to support? All browsers since 2011 support if. In fact, since IE9 hit end of life I don't think there are any supported (as in getting security patches) browsers that don't support classList.

GuillaumeGomez · 2017-04-17T11:16:29Z

That's not up to debate. Docs are supposed to be usable by the maximum number of people, whatever the web browser. It doesn't change much, it just adds a function.

carols10cents · 2017-04-17T14:48:10Z

That's not up to debate. Docs are supposed to be usable by the maximum number of people, whatever the web browser.

@GuillaumeGomez It very much is up to debate-- just like we have tier-1 platforms, we should also have tier-1 web browsers that we spend our time supporting. There are real costs to choosing to support arbitrarily old browsers.

I'm not saying we shouldn't support old browsers, just that we should consider the tradeoffs of doing so.

GuillaumeGomez · 2017-04-17T15:14:02Z

The change to support old browsers is 3 lines long, so I don't understand why there is a debate in here. :-/

alexcrichton · 2017-04-20T15:07:05Z

Randomly assigning @frewsxcv from docs team, mind taking a look and reviewing @frewsxcv?

steveklabnik · 2017-04-20T15:23:03Z

@alexcrichton that's a good assignment; we had a long discussion about this PR in the docs meeting yesterday, sort of a collective review at a high level :)

frewsxcv · 2017-04-20T18:03:46Z

I'm not sure I'm the best person to review this. I'm not really in favor of the removal personally unless we have thorough testing on all the browsers we support, which I don't have time to do.

frewsxcv · 2017-04-20T18:06:51Z

Also, I don't really care that we currently require jQuery. Its existence makes our JavaScript code cleaner and more backwards compatible.

frewsxcv · 2017-04-20T19:45:31Z

src/librustdoc/html/static/main.js

+            var click_func = function(e) {
+                var el = e.target;
+                // to retrieve the real "owner" of the event.
+                while (el.tagName !== 'TR') {


What does this logic correspond to in the old code?

Jquery allows to get the real owner of the event, whereas you have to find yourself normally. So if you bind something on an element, if it has children, then it's certainly one of the children which will "emit" the event.

frewsxcv

Did a quick skim through the code and it seems alright. Considering all the changes that are happening in rustdoc right now, it might be good to hold off on this for a bit until things have settled?

frewsxcv · 2017-04-20T20:02:52Z

src/librustdoc/html/static/main.js

+                link.href = '../' + crates[i] + '/index.html';
+                link.title = rawSearchIndex[crates[i]].doc;
+                link.className = klass;
+                link.innerHTML = crates[i];


Would it be more appropriate to use textContent here instead of innerHTML?

frewsxcv · 2017-04-20T20:06:45Z

src/librustdoc/html/static/main.js

+            var div = document.createElement('div');
+            div.className = 'block ' + shortty;
+            var h3 = document.createElement('h3');
+            h3.innerHTML = longty;


ditto textContents

frewsxcv · 2017-04-20T20:07:04Z

src/librustdoc/html/static/main.js

+                link.href = current.relpath + path;
+                link.title = desc;
+                link.className = klass;
+                link.innerHTML = name;


textContent

GuillaumeGomez · 2017-04-21T16:50:22Z

Did a quick skim through the code and it seems alright. Considering all the changes that are happening in rustdoc right now, it might be good to hold off on this for a bit until things have settled?

I said it on IRC that we should in any case wait for the new beta to land before merging this one. Thanks for your review!

GuillaumeGomez · 2017-04-28T14:59:29Z

it'd be helpful if you didn't squash your commits for now so we can easily see the changes you're making

People seemed to agree on the fact that the removal was almost done so I squashed at this point. My bad for the bad understanding.

GuillaumeGomez · 2017-04-29T09:02:34Z

Squashed and pushed (I replaced innerHTML with textContent).

tafia

Small js comment ...
Feel free not to take them into account!

tafia · 2017-05-02T02:55:24Z

src/librustdoc/html/static/main.js

+            }
+        }
+        return false;
+    }


I am really not a javascript guru, but wouldn't this function be faster and shorter (We probably don't really care but ...).

function hasClass(elem, className) { if (elem && className && elem.className) { return (" " + elem.className + " ").indexOf(" " + className + " ") != -1; } return false; }

Or even faster but more code:

function hasClass(elem, className) { if (elem && className && elem.className) { var elemClass = elem.className; var start = elemClass.indexOf(className); if (start == -1) { return false; } else if (elemClass.length == className.length) { return true; } else { if (start > 0 && elemClass[start - 1] != ' ') { return false; } var end = start + className.length; if (end < elemClass.length && elemClass[end] != ' ') { return false; } return true; } return false; } }

I'll take the second one. Thanks!

tafia · 2017-05-02T03:51:16Z

src/librustdoc/html/static/main.js

+            }
+            elem.className = x.join(' ');
+        }
+    }


similarly this one:

function removeClass(elem, className) { if (elem && className && elem.className) { elem.className = (" " + elem.className + " ") .replace(" " + className + " ", " ") .trim(); } }

GuillaumeGomez · 2017-05-02T10:22:17Z

Updated with @tafia's suggestions.

frewsxcv · 2017-05-04T19:19:07Z

I'm still not a big fan of the jQuery removal, but it sounds like people want to get rid of it. A lot of the logic introduced in this PR is pretty complicated compared to the existing code, which is unfortunate. I just skimmed through and nothing seemed glaringly wrong though. I also tried it out locally and didn't come across any issues in my manual testing. @steveklabnik Any reason to hold off on an r+?

steveklabnik · 2017-05-05T14:07:07Z

As long as we don't regress browsers badly (which I don't think we do), then I'm fine with it.

…

On Thu, May 4, 2017 at 3:19 PM, Corey Farwell ***@***.***> wrote: I'm still not a big fan of the jQuery removal, but it sounds like people want to get rid of it. A lot of the logic introduced in this PR is pretty complicated compared to the existing code, which is unfortunate. I just skimmed through and nothing seemed glaringly wrong though. I also tried it out locally and didn't come across any issues in my manual testing. @steveklabnik <https://github.com/steveklabnik> Any reason to hold off on an r+? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#41307 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABsir3wbZH9BITzD9P2TcpTwxIk8NIvks5r2iSsgaJpZM4M-LJP> .

frewsxcv · 2017-05-05T15:40:38Z

@bors r+

bors · 2017-05-05T15:40:39Z

📌 Commit 6f4c12e has been approved by frewsxcv

…ewsxcv Remove jquery dependency r? @rust-lang/docs Fixes rust-lang#39159.

Rollup of 9 pull requests - Successful merges: #41064, #41307, #41512, #41582, #41678, #41722, #41734, #41761, #41763 - Failed merges:

bors · 2017-05-06T01:46:43Z

⌛ Testing commit 6f4c12e with merge 42a4f37...

bors · 2017-05-06T01:49:04Z

☔ The latest upstream changes (presumably #41773) made this pull request unmergeable. Please resolve the merge conflicts.

@steveklabnik

Fix regression introduced by jQuery removal Fixes rust-lang#42078. Follows rust-lang#41307. r? @steveklabnik cc @frewsxcv

@steveklabnik

Fix regression introduced by jQuery removal Fixes rust-lang#42078. Follows rust-lang#41307. r? @steveklabnik cc @frewsxcv

@steveklabnik

Fix regression introduced by jQuery removal Fixes rust-lang#42078. Follows rust-lang#41307. r? @steveklabnik cc @frewsxcv

@steveklabnik

Fix regression introduced by jQuery removal Fixes rust-lang#42078. Follows rust-lang#41307. r? @steveklabnik cc @frewsxcv

@GuillaumeGomez

…meGomez rustdoc: Fix implementors list javascript * Use a different loop variable, `i` was already taken. This caused missing items in the implementors list. * Use `.getAttribute('href')` rather than `.href` to get the relative URL which is what it needs to actually fix the links. More fallout from rust-lang#41307. r? @GuillaumeGomez

GuillaumeGomez force-pushed the jquery-removal branch 2 times, most recently from 67188bc to c20e8be Compare April 15, 2017 00:07

frewsxcv reviewed Apr 15, 2017

View reviewed changes

GuillaumeGomez force-pushed the jquery-removal branch from c20e8be to 2d9a5f1 Compare April 15, 2017 11:26

jnv reviewed Apr 15, 2017

View reviewed changes

euclio reviewed Apr 15, 2017

View reviewed changes

arielb1 added A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 15, 2017

jnv reviewed Apr 15, 2017

View reviewed changes

GuillaumeGomez force-pushed the jquery-removal branch from 4bf9566 to 952036d Compare April 15, 2017 20:37

GuillaumeGomez force-pushed the jquery-removal branch from 952036d to f747b00 Compare April 17, 2017 17:17

alexcrichton assigned frewsxcv Apr 20, 2017

frewsxcv reviewed Apr 20, 2017

View reviewed changes

GuillaumeGomez force-pushed the jquery-removal branch from 30d9891 to 78137f4 Compare April 29, 2017 09:02

GuillaumeGomez added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 1, 2017

tafia reviewed May 2, 2017

View reviewed changes

GuillaumeGomez force-pushed the jquery-removal branch from 78137f4 to 8c66bbb Compare May 2, 2017 10:21

Remove jquery dependency

6f4c12e

GuillaumeGomez force-pushed the jquery-removal branch from 8c66bbb to 6f4c12e Compare May 2, 2017 11:54

frewsxcv approved these changes May 5, 2017

View reviewed changes

frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 5, 2017

Rollup merge of rust-lang#41307 - GuillaumeGomez:jquery-removal, r=fr…

31a4d73

…ewsxcv Remove jquery dependency r? @rust-lang/docs Fixes rust-lang#39159.

frewsxcv mentioned this pull request May 5, 2017

Rollup of 9 pull requests #41773

Merged

bors added a commit that referenced this pull request May 5, 2017

Auto merge of #41773 - frewsxcv:rollup, r=frewsxcv

42a4f37

Rollup of 9 pull requests - Successful merges: #41064, #41307, #41512, #41582, #41678, #41722, #41734, #41761, #41763 - Failed merges:

bors merged commit 6f4c12e into rust-lang:master May 6, 2017

GuillaumeGomez deleted the jquery-removal branch May 6, 2017 11:21

This was referenced May 18, 2017

Nightly documentation is broken #42078

Closed

Fix regression introduced by jQuery removal #42080

Merged

frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 18, 2017

Rollup merge of rust-lang#42080 - pravic:jquery-removal-fix, r=frewsxcv

fdb21b6

Fix regression introduced by jQuery removal Fixes rust-lang#42078. Follows rust-lang#41307. r? @steveklabnik cc @frewsxcv

ollie27 mentioned this pull request May 19, 2017

rustdoc: Fix implementors list javascript #42096

Merged

Remove jquery dependency #41307

Remove jquery dependency #41307

Conversation

GuillaumeGomez commented Apr 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

projektir commented Apr 15, 2017

GuillaumeGomez commented Apr 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

euclio commented Apr 15, 2017

GuillaumeGomez commented Apr 15, 2017

jnv Apr 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

projektir commented Apr 15, 2017

johansigfrids commented Apr 17, 2017

GuillaumeGomez commented Apr 17, 2017

johansigfrids commented Apr 17, 2017

GuillaumeGomez commented Apr 17, 2017

carols10cents commented Apr 17, 2017

GuillaumeGomez commented Apr 17, 2017

alexcrichton commented Apr 20, 2017

steveklabnik commented Apr 20, 2017

frewsxcv commented Apr 20, 2017

frewsxcv commented Apr 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frewsxcv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Apr 21, 2017

GuillaumeGomez commented Apr 28, 2017

GuillaumeGomez commented Apr 29, 2017

tafia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented May 2, 2017

frewsxcv commented May 4, 2017

steveklabnik commented May 5, 2017 via email

frewsxcv commented May 5, 2017

bors commented May 5, 2017

bors commented May 6, 2017

bors commented May 6, 2017

GuillaumeGomez commented Apr 14, 2017 •

edited

Loading

jnv Apr 15, 2017 •

edited

Loading