Refactored doc-switcher.js
#1835
Conversation
djangoproject/static/js/app.js
Outdated
There was a problem hiding this comment.
Should this file be renamed? I wasn't sure what to call it.
There was a problem hiding this comment.
I don't have strong feelings, but app.js is a bit meaningless to me, yeah. How about djangoproject.js?
There was a problem hiding this comment.
Great! I just pushed the change in f3ab40d.
djangoproject/static/js/app.js
Outdated
| @@ -0,0 +1,5 @@ | |||
| document.querySelectorAll('.doc-switcher li.current').forEach(function (el) { | |||
There was a problem hiding this comment.
This will run on all pages, but in my testing, it seems to be about twice as fast as the old hasClass method from main.js. So I think we're safe here.
| // Make version switcher clickable for touch devices | ||
|
|
||
| self.switchers.find('li.current').on('click', function () { | ||
| $(this).closest("ul").toggleClass('open'); |
There was a problem hiding this comment.
I used the parent element in the new code instead of trying to find the "closest" ul element. The li elements we are looking for should always be direct children of the ul element that needs the class.
|
I suppose we could try to replace this JavaScript with a |
djangoproject/static/js/app.js
Outdated
| // Toggle persistent display of documentation version and language options if | ||
| // the element is clicked |
There was a problem hiding this comment.
Should we keep a comment like this? It may be valuable to have a comment for each bit of behavior that goes into this file. I don't feel strongly about it.
There was a problem hiding this comment.
Comments can be useful for developers but increases the size of the website and page lightness is a priority if we want to be accessible also for people with old devices or slow connection.
What do you think about having a minified version of the JavaScript file with all comments stripped?
There was a problem hiding this comment.
I wonder if we should start getting refactors in to see how much JavaScript we're going to end up with before making the decision to minify it. I'm confident that it's going to be way less than what's currently on production, and that's not minified.
Just to be clear, are you saying that we should set up django-compressor for app.js?
Also, I'm fine just not having the comment too.
There was a problem hiding this comment.
It's an interesting consideration about size of js and slow connections.
My opinion on this is that Adam has removed so many lines of js that a few comments are allowed 😁
django-compressorwould be the solution to this for me, but that's out of scope for this PR I would say.
There was a problem hiding this comment.
Yes, an amazing job has been done here. We can think about minified JS in another PR
|
I've only skimmed the PR, but it seems pretty good so far. Thanks for working on this! 🎸 Could we start by setting up a formatter/linter/ci for js before we start the refactors? |
|
@bmispelon Sure. See #1838. |
- Simplified code - Stopped using jQuery - Moved refactored code to `app.js` This is the first in a series of patches that will move JavaScript code out of `require.js` modules and into a single file while also refactoring. This patch should bring no user-facing changes.
adamzap
force-pushed
the
refactor-doc-switcher-js
branch
from
f9c0811 to
249c1e6
Compare
|
This has been rebased to get 6dc8f41 under it. Let me know about renaming |
|
@bmispelon Thanks! I pushed one tiny edit to the comment! |
app.jsThis is the first in a series of patches that will move JavaScript code out of
require.jsmodules and into a single file while also refactoring.This patch should bring no user-facing changes.
To test, ensure that the functionality of this component on documentation pages still works as expected:
If the "Language" or "Documentation version" elements are clicked, it should toggle them remaining open when not hovered over. They will get a green border when activated.
Part of #1827