Problem/Motivation
As mentioned in the parent issue #3238306: [META] Where possible, refactor existing jQuery uses to vanillaJS to reduce jQuery footprint, we are working towards reducing our jQuery footprint. One of the ways to accomplish this is to reduce the number of jQuery features used in Drupal core. We have added eslint rules that identify specific features and fail tests when those features are in use.
There are (or will be) individual issues for each jQuery-use eslint rule. This one is specific to jquery/no-css, which targets the jQuery css function.
Steps to reproduce
Proposed resolution
Remaining tasks
- In
core/.eslintrc.jquery.jsonChange"jquery/no-css": 0,to"jquery/no-css": 2,to enable eslint checks for uses of jQuerycss(). With this change, you'll be able to see uses of the undesirable jQuery feature by runningyarn lint:core-js-passingfrom thecoredirectory - Add the following lines to
core/scripts/dev/commit-code-check.shso the DrupalCI testing script can catch this jQuery usage on all files, not just those which have changed
# @todo Remove the next chunk of lines before committing. This script only lints # JavaScript files that have changed, so we add this to check all files for # jQuery-specific lint errors. cd "$TOP_LEVEL/core" node ./node_modules/eslint/bin/eslint.js --quiet --config=.eslintrc.passing.json . CORRECTJQS=$? if [ "$CORRECTJQS" -ne "0" ]; then # No need to write any output the node command will do this for us. printf "${red}FAILURE ${reset}: unsupported jQuery usage. See errors above." STATUS=1 FINAL_STATUS=1 fi cd $TOP_LEVEL # @todo end lines to removeAdd the block about 10 lines before the end of the file, just before
if [[ "$FINAL_STATUS" == "1" ]] && [[ "$DRUPALCI" == "1" ]]; then, then remove it once all the jQuery uses have been refactored. - If it's determined to be feasible, refactor those uses of jQuery
css()to use Vanilla (native) JavaScript instead.
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #44 | 3238308-44.patch | 19.11 KB | mstrelan |
Issue fork drupal-3238308
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
bnjmnmComment #4
bnjmnmComment #5
bnjmnmComment #6
bnjmnmThe
Custom Commands Failedin the most recent commit is intentional. This means the DrupalCI testing script is successfully catching the uses of jQuerycssthat should be refactored to use Vanilla JS. Once those are all addressed, there should no longer be aCustom Commands FailedAND the extra lines incore/scripts/dev/commit-code-check.shchecking for this (as mentioned in issue summary) can be removed.TLDR: tests will now fail in this MR until
jQuery().css()uses are vanillified. Happy refactoring!Comment #8
mstrelan commentedComment #9
rikki_iki commentedComment #10
mstrelan commentedI'm at a loss here. All tests are passing in https://www.drupal.org/pift-ci-job/2247858 but with one instance of $.css() remaining. There are 14 fails in https://www.drupal.org/pift-ci-job/2248118 but these are passing locally for me.
Comment #12
mstrelan commentedNote to self since I don't know when I'll come back to this:
Can reproduce the test fails but adding a 100ms wait before the first fail.
Comment #13
mstrelan commentedComment #15
yogeshmpawarCS errors.
Comment #17
mstrelan commentedComment #19
mstrelan commentedGitlab won't let me create a new branch from 10.1.x so uploading a patch. It's a lot simpler now that we removed ckeditor, color, quickedit and bartik, and we don't have to transpile es6.
Comment #20
nod_When there is an object in the css method we can use object assign to have a similar DX:
Comment #21
mstrelan commentedFixed linting and addressed feedback in #20, assuming this is only necessary where styles are applied to multiple properties. There were some instances where an object was used with a single property, but I have left those as the were in #19.
Comment #22
mstrelan commentedI don't know why the nightwatch test failed, so reverting that one to see if it comes back green.
Comment #23
mstrelan commentedOh, I accidentally started writing Object.apply instead of Object.assign. Let's try this again.
Comment #24
mstrelan commentedOne more instance I forgot to update. It must be Friday afternoon.
Comment #25
smustgrave commentedSo wasn't super clear how to best test this one.
But I applied patch #24 locally and for a few days I've been using it locally.
Created views
Created content types
Tested on mobile
And nothing seems to break and didn't notice any errors in the console.
If better way please let me know.
Comment #28
mstrelan commentedRandom fail, setting back to RTBC.
Comment #29
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #30
nod_maybe this one needs a bit more, jquery handles kebab-case to camelCase transformation of css rules.
At least in core there are two uses of the CssCommand that uses kebab-case.
Since this code is aimed at backend folks and all the jQuery niceties are expected, let's exclude this one from the list. We can handle that when we uncouple ajax.js from jquery.
I mean let's use .each() since we're working with jquery elements, when converting the selector to vanilla we'll be able to directly use .forEach.
doesn't seem related to .css() functions
Not needed anymore, it's automatic when changing eslint config now.
Comment #31
nod_Comment #32
mstrelan commentedComment #33
mstrelan commentedComment #34
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #35
nod_We need to add a comment to disable the eslint rule for this one use in ajax.js
Comment #36
mstrelan commentedComment #37
mstrelan commentedI forgot how to jquery.
Comment #38
smustgrave commentedupdate looks good. Hopefully can get this in early for 10.2
Comment #39
larowlanWe have some cases where we use Object.entries.forEach and others where we use Object.assign - is there a reason?
Per #20 I think the
.assignfeels nicer.any reason we don't use
document.bodyhere?$.each()here, which feels odd given we're trying to remove usage of jQuery, ah but #30.2 details whyCouple of questions, putting back to NR for the first two
Comment #40
mstrelan commentedGood points, I think they were just oversights. Updated patch addresses 39.1 and 39.2.
Comment #41
mstrelan commented#39.2 Let's just revert back to the original jquery selectors, changing them is not really in scope
Comment #42
smustgrave commentedDo we want a follow up for 39.2?
Comment #43
larowlanNeeds a reroll after it .is issue went in
Conflict in
core/modules/views_ui/js/views-admin.jsComment #44
mstrelan commented#42 I think there is (or should be) a child issue of #3238306: [META] Where possible, refactor existing jQuery uses to vanillaJS to reduce jQuery footprint to replace all use of jquery selectors with native equivalents.
Re-rolled #41 to fix up #43.
Comment #45
smustgrave commentedLGTM
Comment #46
larowlanComment #48
larowlanCommitted 6a43b51 and pushed to 11.x. Thanks!
Comment #50
iberezovchuk commentedAfter this refactoring tableheader.js stopped adding correct width to sticky-header.
Comment #51
ljwilson commentedWe just migrated a production site from 10.1.8 to 10.2.6, and noticed for one of our custom blocks the contextual nested links were overlaying on top of each other. I tracked it down to this change:
core/modules/contextual/js/contextual.js
I asked Chat-GPT what the difference might be, and it recommended adding 'px' since the version in 10.1.8 probably interpreted it as pixels but the 10.2.x version does not. Adding the units of 'px' fixed the problem. My change below: