Refactor (if feasible) uses of the jQuery show and hide function to use vanillaJS

I think this should be combined with the hide() function and subsequent jquery/no-hide, which I couldn't find an existing issue for. Possibly also need to consider fadeIn / fadeOut / fadeToggle / slideUp / slideDown / slideToggle at the same time.

Here, i am not able to figure out one remaining pipeline error which is Spell-check can anyone help to figureout what might be exactly the error is ?

You need to push an up-to-date 11.x branch to your issue fork, I think that should fix it.

@mstrelan yes i rebased my branch with current updated 11.x still its showing the error..

hi @shubh511

It seems like your branch is 54 commits behind the base 11.x branch, to rebase please follow these steps https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... →

Added several feedbacks. However MR needs huge work still

Acknowledged all the feedbacks on MR..

All feedback have been addressed..

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.

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.

@mstrelan do you still want to combine this with other rules? I don't see your comment in #7 being addressed. I don't see a reason to combine, personally I think it makes more sense to keep this separate so scope is more limited.

Another reason is this looks pretty close to approaching ready.

I did a review of core and don't see any other show methods missing (the linter should catch that but I saw remaining steps mentioned changing the commit check script and I wasn't sure if that was made).

Remaining show calls fell into three categories that I could see:
1. asset vendor files
2. element show methods (dialog and details)
3. one call from context in /core/misc/machine-name.js

Category 1 and 2 are out of scope of this issue as far as I can tell.

I think category 3 does need to be addressed, but I am not confident. I've copied the relevant snippet below:

if (machine !== '') {
        if (machine !== settings.replace) {
          data.$target[0].value = machine;
          data.$preview.html(
            settings.field_prefix +
              Drupal.checkPlain(machine) +
              settings.field_suffix,
          );
        }
        data.$suffix.show();
}

Suffix is defined above const $suffix = $context.find(options.suffix);
and $context is const $context = $(context);

Hey @nicxvan your comment has been addressed around the third category which was not captured by linter..

This looks good to me and tests are still passing.

I'm not sure a completed rule conversion should be delayed due to wanting to combine other rules.

I'm not overly fussed, but we now have code that looks like this:

// Set the machine name to the transliterated value.
if (machine !== '') {
  // ...
  data.$suffix[0].style.display = 'block';
} else {
  data.$suffix.hide();
  // ...
}

Where the truthy part of the if condition sets the style directly but the falsey part uses jquery hide.

Besides, I haven't looked thoroughly, but how do we know 'block' is the right value, maybe it should be 'inline-block' or 'inline'? IIRC jquery takes care of that.

You did recall correctly:

From the docs:
The matched elements will be revealed immediately, with no animation. This is roughly equivalent to calling .css( "display", "block" ), except that the display property is restored to whatever it was initially. If an element has a display value of inline, then is hidden and shown, it will once again be displayed inline.

https://api.jquery.com/show/

so what exactly need to change here ?

I think that .show() just uses a class to hide and show items so the display type of the elements is preserved.

The current conversion will convert a flex, inline or inline-block element to a block element.

I'm not sure of the solution, I'm not even sure this would be considered a regression for the uses in this PR, but it should be addressed.

I tend to agree the display value set to block is a problem. I don't have an elegant solution here using a class or even a data attribute (maybe even the hidden attribute?) would allow to bypass the problem with display block.

If there are tricky bits we can have followups to make it easier to get done/reviewed

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

Can you confirm what else you changed?

I'm not sure you've addressed the feedback where this functionally changes how it works I think for .show vs this solution.

It looks like classes are the appropriate way to manage this. So instead of using display none and display block we should add and remove the hidden class:

https://browse-tutorials.com/snippet/vanilla-javascript-equivalent-jquer...

@shubh_ to be explicit since you've still got yourself assigned, I think the specific ask is to do the following:

1. Read the linked recommendations: https://browse-tutorials.com/snippet/vanilla-javascript-equivalent-jquer...
2. Update the show method that already exists in this MR to look for and remove the hidden class. Using this guide: https://www.drupal.org/docs/getting-started/accessibility/hide-content-p... →
3. Find all instances of .hide() and change them to use native javascript and add the hidden class using the guide in 2.

I think that follows best practice recommendations for how to convert from jquery to native JS in this case.

I got the recommendation from @brianperry