Problem/Motivation

The eslint rules used in #3239134: Refactor (if feasible) uses of the jQuery val function to use VanillaJS is not catching all usage of .val() it is still used in the following files: 

core/misc/states.es6.js
        return this.val() === '';
          return this.filter(':checked').val() || false;
        return this.val();
          return this.filter(':checked').val() || false;
        return this.val();

core/modules/ckeditor/js/models/Model.es6.js
        this.get('$textarea').val(

core/modules/color/preview.es6.js
          form.find('.color-palette input[name="palette[base]"]').val(),
          form.find('.color-palette input[name="palette[text]"]').val(),
          form.find('.color-palette input[name="palette[link]"]').val(),
            .val(),
            .val(),

core/modules/filter/filter.filter_html.admin.es6.js
        this.$allowedHTMLFormItem.val(
        this.$allowedHTMLFormItem.val(this._generateSetting(this.userTags));

// This one is a legitimate use
core/modules/quickedit/js/views/EditorView.es6.js
            .val(value);

core/themes/claro/js/vertical-tabs.es6.js
        .val(this.details.attr('id'));

We should also open an issue in the upstream project.

Steps to reproduce

Proposed resolution

Update states.js
Do not change jquery.form.js because it is excluded from eslint

Remaining tasks

Open an issue in the upstream project.

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#12 3259381-nr-bot.txt1.35 KBneeds-review-queue-bot

Issue fork drupal-3259381

Command icon 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:

About issue forks

Comments

nod_ created an issue. See original summary.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Issue summary: View changes
Parent issue: #3239123: Refactor (if feasible) uses of the jQuery text function to use vanillaJS » #3239134: Refactor (if feasible) uses of the jQuery val function to use VanillaJS

kmonahan made their first commit to this issue’s fork.

kmonahan opened merge request !2250

kmonahan’s picture

kmonahan commented
Status: Active » Needs review

Opened https://git.drupalcode.org/project/drupal/-/merge_requests/2250 with updates to the occurrences of val() listed in the issue description.

kmonahan’s picture

kmonahan commented
Status: Needs review » Needs work

Some failing tests to look at.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Related issues: +#3238306: [META] Where possible, refactor existing jQuery uses to vanillaJS to reduce jQuery footprint

Version: 10.0.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

thebumik’s picture

thebumik commented
Assigned: Unassigned » thebumik

thebumik opened merge request !7390

thebumik’s picture

thebumik commented
Status: Needs work » Needs review

Replaced jQuery val() methods with VanillaJS. Updated the fork with the latest HEAD and PR is created against 11. x.
Please review!

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3259381-nr-bot.txt1.35 KB

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

nod_’s picture

nod_
Primary language French
Location Lille
commented

few comments on the MR, thanks for picking it up!

nod_’s picture

nod_
Primary language French
Location Lille
commented

I think you reverted a bit too much code in jquery.form.js, we still need to remove the calls to .val() maybe I wasn't clear, sorry.

nod_’s picture

nod_
Primary language French
Location Lille
commented

Had a closer look. Couple of things:

  1. Let's drop the changes to filter.filter_html.admin.js: The whole thing is deprecated and will be removed in D11. #2567801: Deprecate core/modules/editor/js/editor.admin.js JS APIs in Drupal 10, for removal in Drupal 11
  2. In the same spirit, since jquery.form.js is excluded from eslint, let's remove changes to this one as well. It's a temporary fork until we get rid of it and this one require jQuery to work at all so no sense in removing the dependency on it.
  3. The states test failure is legit, the new code not support select with multiple values, jQuery return an array with all the selected options, .value only return the first value. We need to work around that somehow.

Sorry for making you add/remove changes to specific files, I changed my mind after spending a bit more time with the code.

yash.rode made their first commit to this issue’s fork.

Utkarsh_33 made their first commit to this issue’s fork.

utkarsh_33’s picture

utkarsh_33 commented
Assigned: thebumik » Unassigned
Status: Needs work » Needs review

The test are fixed now.In response to

Let's drop the changes to filter.filter_html.admin.js: The whole thing is deprecated and will be removed in D11. #2567801: Deprecate core/modules/editor/js/editor.admin.js JS APIs in Drupal 10, for removal in Drupal 11

06c9630b

commit removes the whole file but as far as i understood from the comment is that we just wanted to revert the changes from that file.It would be helpful if @nod_ can verify the change.
Marking it needs review as rest everything is addressed apart from the changes in 06c9630b commit.

smustgrave changed the visibility of the branch 3259381-follow-up-for-jquery to hidden.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Still not a huge fan of [0]. personally but it was used in the previous commit so would assume fine here.

quietone’s picture

quietone commented
Title: Follow-up for jQuery val replacement » Convert remaining jQuery val replacement not found by eslint
Issue summary: View changes
Issue tags: +Needs followup

I read the IS, the comments and the MR. I have updated the title to reflect what is being fixed here and why.

This still needs a followup per the issue summary.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Needs work

I'd like to have a function to avoid duplicating the code in two places here. The function can be local to this file, no need to add something to the global Drupal object

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.