-
-
Notifications
You must be signed in to change notification settings - Fork 967
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Proposal for further front-end cleanups #1827
Comments
Thanks, @adamzap, for all your work and effort to modernize the frontend of the website. All your proposal seems ok for me. I would rather remove jQuery totally than upgrade it, also if this requires more PR. |
TLDR: Yes, yes, yes, yes, and yes! 😁 One thing I'd like to mention first is that as far as I'm concerned, "maintainers" include you at this point. In fact if you're OK with that, I'd like to add you to the maintainer group of the repository. It shouldn't change the day-to-day work, but there would be a few extra features like assigning you to tickets/prs, you being able to manage labels, ... As for your proposal, I'm in favor of all the points, and I've put details under each heading. Logistically, I think we can use this ticket as a progress tracker of sort while you sort out individual PRs for the different sections. If we find that things are getting hard to keep track of, we can open separate tickets for some subtasks (I'm thinking for example of the jquery removal, which might be a longer running task than the others). Remove
|
@bmispelon It's very kind of you to offer me that role. I accept! Thanks for the feedback on my cleanup suggestions. I will take some time to think about it. While I was reading your comment, another idea emerged: what if I go through the JavaScript files in the
I could try with one of the simpler files first and see if we like the pattern. In my mind, this would clear away the outdated parts so that we can see what kind of complexity we're really dealing with. Testing the changes should be easier too since they will be deep instead of broad. |
I agree with everything @bmispelon wrote, and I liked your plan. I would just add that in reviewing the JavaScript files to modernize them, and at the same time remove old libraries or JQuery, I would also try to understand if part of the JavaScript code can be removed because it was initially useful to give dynamism to the pages, a goal that can now be achieved by taking advantage of the support of modern browsers to HTML5 (e.g.: tag details). |
@pauloxnet Yes, I'm hoping at least some of the JavaScript can be removed altogether! |
A few questions as I start this process:
Original Filedefine([
'jquery' //requires jquery
], function( $ ) {
var DocSwitcher = function(switchers) {
this.switchers = $(switchers);
this.init();
};
DocSwitcher.prototype = {
init: function(){
var self = this;
$(document).ready(function () {
// Make version switcher clickable for touch devices
self.switchers.find('li.current').on('click', function () {
$(this).closest("ul").toggleClass('open');
});
});
}
};
// Export a single instance of our module:
return new DocSwitcher('.doc-switcher');
}); Rewritten File with Enclosing ScopeWe shouldn't need to check that the document is ready/loaded because we'll include all JavaScript modules just before the closing (function () {
document.querySelectorAll('.doc-switcher li.current').forEach(function (el) {
el.addEventListener('click', function () {
this.parentElement.classList.toggle('open');
});
});
})(); Rewritten File with No Enclosing ScopeThere is no global variable leakage, so I don't think we need the enclosing scope. document.querySelectorAll('.doc-switcher li.current').forEach(function (el) {
el.addEventListener('click', function () {
this.parentElement.classList.toggle('open');
});
}); Rewritten File with No Enclosing Scope with DOM UtilitiesWe could add a few lines to const $ = document.querySelector; // Defined in `base.html`
const $$ = document.querySelectorAll; // Defined in `base.html`
EventTarget.prototype.on = EventTarget.prototype.addEventListener; // Defined in `base.html`
$$('.doc-switcher li.current').forEach(function (el) {
el.on('click', function () {
this.parentElement.classList.toggle('open');
});
}); |
`bower` is maintained but not recommended for use by its developers. Our `bower.json` file now only includes two dependencies: `jquery` and `jquery-flot`. At this point, `bower` is not doing much to serve the project. Refs #1827
`bower` is maintained but not recommended for use by its developers. Our `bower.json` file now only includes two dependencies: `jquery` and `jquery-flot`. At this point, `bower` is not doing much to serve the project. This is a redo of 98e20a0 which was reverted because it led to errors in production when running collectstatic. Refs #1827
@bmispelon The more I look at the code, the more I like your idea of having only one JavaScript file that's included on every page. Good insight there! It's just not very much code. I can do the Stripe parts last to see if they are worth putting in a separate file. |
I don't have a preference, as long as it's enforced by an autoformatter and checked in CI. I don't think the js code has been actively changed in years, so at this point I think you get to decide what you like better 😁
My personal order of preference (from most to least preferred) would be: 2 (unless there's things that might leak out of scope), 1 then 3 (the dollar-based names seem a big gimmicky to me and I like plain js). But again I don't have a very strong preference, so go with what feels right to you. |
I totally agree
Same for me |
Thanks for the feedback. I'll go with two spaces and the second style. I won't be offended that no one likes my three-line JavaScript framework! 😉 |
I’d rather stay out of any decision, but would recommend considering ES modules with dynamic |
@thibaudcolas Interesting! I don't have any experience with that approach, but I can start looking into it if that would be the collective desire. The approach I started with in #1835 that was suggested above seems simple to me, but I am biased toward "classic" JavaScript. |
This patch removes the global `djangoproject/static/*` exclude rule in `.pre-commit-config.yaml` to enable `prettier` to run on this project's JavaScript files. The `djangoproject/static/js/lib/` directory is now excluded in the `prettier` hook because there is no value in reformatting a minified JavaScript file. I also added a `.prettierrc` file to the root of the project to control JavaScript formatting. Refs #1827
- 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. Refs #1827
- Simplified code - Stopped using jQuery - Moved refactored code to `djangoproject.js` This patch should bring no user-facing changes. Refs django#1827
- Simplified code - Stopped using jQuery - Moved refactored code to `djangoproject.js` This patch should bring no user-facing changes. Refs #1827
- Simplified code - Stopped using jQuery - Moved refactored code to `djangoproject.js` This patch should bring no user-facing changes. Refs django#1827
- Simplified code using a combination of CSS and JavaScript - Stopped using jQuery - Moved refactored code to `djangoproject.js` This patch should bring no user-facing changes. Refs django#1827
- Simplified code - Stopped using jQuery - Moved refactored code to `djangoproject.js` This patch should bring no user-facing changes. Refs django#1827
I'm curious if the maintainers would accept pull requests for the following front-end changes. Some of these ideas are more opinionated or need more guidance than my previous PRs, so I wanted to get feedback first.
Remove(#1830/#1832)bower
As its website says,
bower
is maintained but not recommended for use by its developers. Ourbower.json
file now only includes two dependencies:jquery
andjquery-flot
.Instead of using
bower
, could we instead putjquery.min.js
andjquery-flot.min.js
directly indjangoproject/static/js/lib/
? This would include removing thejquery
andjquery-flot
directories.Remove
require.js
require.js
had a small security release in July 2024, but the release before that was in 2018, and the project is not under active development. I'm still trying to understand the full influence thatrequire.js
is having on this project, but removing it would require at least the following changes in addition to removing the library itself:require.js
in thebase.html
templatedefine
calls in our JavaScript modules (example)main.js
<script>
tags to the templates where needed. Adding a new template block for JavaScript includes would also be a good idea. This approach is simple but is riskier and requires more work and testing, but it's the one I would choose.<script>
tags to the DOM based on it. This approach requires less changes and may be a good way just to removerequire.js
without broader changes.Which approach would be preferred here, or is there a better way than what I presented above?
Remove Boilerplate in Many JavaScript Modules
Many of our JavaScript modules define an enclosing function then add an
init
method to its prototype. This is where the actual logic lives. It seems to me that we could decompose these and just run everything in the top-level enclosing function. Perhaps the current pattern is related torequire.js
?Could these modules be simplified, or is this kind of refactor too unnecessary?
Upgrade or Remove
jQuery
We're currently using jQuery v1.11.2, and the latest release is v3.7.1. Jumping ahead through ten years of releases could be a pain, but I think it's worth it if jQuery is valued in this project.
Another option would be to remove
jQuery
instead. We could convert one module at a time to native browser APIs before its removal. Doing this would probably force a replacement ofjquery.flot
for thedashboard
app.Integrate a JavaScript Linter/Formatter
I'd be willing to work on this, but I'd want guidance on which tool to use and the settings for it.
Thanks for considering these!
The text was updated successfully, but these errors were encountered: