Refactor (if feasible) uses of the jQuery css function to use Vanilla/native

mstrelan → closed merge request !1879

Gitlab 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.

When there is an object in the css method we can use object assign to have a similar DX:

$something.css({left:0, right:0, top:0});

Object.assign(something.style, {left:0, right:0, top:0});

Fixed 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.

I don't know why the nightwatch test failed, so reverting that one to see if it comes back green.

Oh, I accidentally started writing Object.apply instead of Object.assign. Let's try this again.

One more instance I forgot to update. It must be Friday afternoon.

So 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.

The last submitted patch, 24: 3238308-10-1-24.patch, failed testing. View results →

Random fail, setting back to RTBC.

The 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.

1. +++ b/core/misc/ajax.js
   @@ -1480,7 +1480,11 @@
   -      $(response.selector).css(response.argument);
   

   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.

2. +++ b/core/misc/progress.js
   @@ -74,9 +74,9 @@
   +          [].forEach.call($(this.element).find('div.progress__bar'), (el) => {
   
   +++ b/core/misc/tabledrag.js
   @@ -401,9 +401,13 @@
   +    [].forEach.call($tables.find('.tabledrag-hide'), (el) => {
   ...
   +    [].forEach.call($tables.find('.tabledrag-handle'), (el) => {
   
   @@ -418,9 +422,13 @@
   +    [].forEach.call($tables.find('.tabledrag-hide'), (el) => {
   ...
   +    [].forEach.call($tables.find('.tabledrag-handle'), (el) => {
   
   +++ b/core/modules/views_ui/js/dialog.views.js
   @@ -14,7 +14,12 @@
   +      [].forEach.call($scroll, (el) => {
   
   @@ -24,8 +29,12 @@
   +      [].forEach.call($modal, (el) => {
   

   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.

3. +++ b/core/modules/toolbar/js/views/ToolbarVisualView.js
   @@ -79,17 +79,11 @@
   +        const height = toolbarTabOuterHeight + toolbarTrayHorizontalOuterHeight;
   +        this.model.set('height', height);
   

   doesn't seem related to .css() functions

4. +++ b/core/scripts/dev/commit-code-check.sh
   @@ -495,6 +495,22 @@
   +# @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 remove
   +
   

   Not needed anymore, it's automatic when changing eslint config now.

The 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.

We need to add a comment to disable the eslint rule for this one use in ajax.js

I forgot how to jquery.

update looks good. Hopefully can get this in early for 10.2

1. +++ b/core/misc/tableheader.js
   @@ -245,12 +242,15 @@
   +        Object.entries(css).forEach(([property, value]) => {
   +          this.$stickyTable[0].style[property] = value;
   +        });
   
   @@ -318,14 +317,17 @@
   +            Object.assign($stickyCell[0].style, {
   +              width: window.getComputedStyle($that[0]).width,
   +              display,
   

   We 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 .assign feels nicer.

2. +++ b/core/modules/toolbar/js/views/ToolbarVisualView.js
   @@ -84,12 +84,12 @@
   +        document.querySelector('body').style.paddingTop = `${this.model.get(
   +          'height',
   

   any reason we don't use document.body here?

3. We're adding quite a few new uses of $.each() here, which feels odd given we're trying to remove usage of jQuery, ah but #30.2 details why

Couple of questions, putting back to NR for the first two

Good points, I think they were just oversights. Updated patch addresses 39.1 and 39.2.

#39.2 Let's just revert back to the original jquery selectors, changing them is not really in scope

Do we want a follow up for 39.2?

Needs a reroll after it .is issue went in

Conflict in

core/modules/views_ui/js/views-admin.js

#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.

LGTM

Committed 6a43b51 and pushed to 11.x. Thanks!

Automatically closed - issue fixed for 2 weeks with no activity.

After this refactoring tableheader.js stopped adding correct width to sticky-header.

We 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:

diff --git a/core/modules/contextual/js/contextual.js b/core/modules/contextual/js/contextual.js
index 52fb138e9..324fe7766 100644
--- a/core/modules/contextual/js/contextual.js
+++ b/core/modules/contextual/js/contextual.js
@@ -72,7 +72,7 @@
 
       // Adjust nested contextual link's position.
       $nestedContextual[0].style.top =
-        $nestedContextual.position().top + height;
+        $nestedContextual.position().top + height + 'px';
     }
   }