Refactor/update/jquery #2022
Refactor/update/jquery #2022
Conversation
|
You will most likely have to follow this guidance, in particular using Also, the structure of these commits is chronological, in dear need of becoming logical instead. Keep an eye on reviewability. Hiding the change that drops |
RafaelJohn9
force-pushed
the
refactor/update/jquery
branch
from
b997769 to
3f3ec70
Compare
RafaelJohn9
force-pushed
the
refactor/update/jquery
branch
from
3f3ec70 to
cb66948
Compare
|
hey @dscho , on |
Signed-off-by: rafaeljohn9 <[email protected]>
RafaelJohn9
force-pushed
the
refactor/update/jquery
branch
from
db70fe3 to
a98da04
Compare
That's excellent information, which dearly wants to go into the commit message. |
There was a problem hiding this comment.
I am still convinced that you will most likely have to follow this guidance, in particular using jquery-migrate.
Either you did (but didn't talk about it in the commit messages, which you should, though), or you didn't (but you should).
Speaking of commit messages: For something as consequential as a JQuery update from an ancient version to a current one, you definitely need to up your commit message writing fu.
Please follow the guidance in https://github.blog/2022-06-30-write-better-commits-build-better-projects/ to improve it, in particular with a strong focus on this part:
| What you’re doing | Why you’re doing it | |
|---|---|---|
| High-level (strategic) | Intent (what does this accomplish?) | Context (why does the code do what it does now?) |
| Low-level (tactical) | Implementation (what did you do to accomplish your goal?) | Justification (why is this change being made?) |
| onPopState(function() { | ||
| onPopState(function () { |
There was a problem hiding this comment.
That's not updating a deprecated function call, but it changes the code style to disagree with the remainder of the file.
| return $(window).bind('popstate', function(event) { | ||
| var section; | ||
| return $(window).on('popstate', function () { |
There was a problem hiding this comment.
Can you find some documentation about this? If so, that would be good information for the commit message.
Also: Does this new call work with the outdated JQuery version that git-scm.com currently has? If so, this commit needs to come before the upgrade to the newer JQuery.
| $(window).scroll(function() { | ||
| $(window).on('scroll', function () { | ||
| $(this).scrollTop() > 150 | ||
| ? $('#scrollToTop').fadeIn() | ||
| : $('#scrollToTop').fadeOut(); | ||
| }); | ||
| $('#scrollToTop').click(function(e) { | ||
| $('#scrollToTop').on('click', function (e) { |
There was a problem hiding this comment.
I see a lot of $(...).<something>(...) -> $(...).on('<something>, ...) changes here. Do they all belong to the same class of deprecation, logically? If so, it's fine to cluster them in one large commit. Otherwise it makes reviewing easier (and hence bug catching, too, which is what reviewing is all about) to split the changes up by logical topic (which would then need to be described in the commit message, accompanied by references you can find).
| //= require jquery-1.7.1.min | ||
| //= require jquery-ui-1.8.18.custom.min | ||
| //= require jquery-3.7.1.min | ||
| //= require jquery-ui-1.14.1.min |
There was a problem hiding this comment.
It does not make much sense to split the JQuery version updates in application.js from the ones in the header.
I am glad that you dropped the unrelated change from this commit where jquery.defaultvalue was dropped. But it seems you no longer drop this altogether? Is it still supported? Does it support the newest JQuery version?
Okay, after mulling it over a bit and |
Changes
Context