Remove jquery.once dependency.

Created on 12 June 2023, over 1 year ago
Updated 14 March 2024, 8 months ago

Problem/Motivation

jquery.once library is removed from Drupal 10 core, so this module doesn't work with Drupal 10.

Steps to reproduce

Install it on Drupal 10 site and check that it doesn't work.

Proposed resolution

Remove jquery.once and update code.

Remaining tasks

  • Update responsive_menus_google_nexus.js
  • Update responsive_menus_mean_menu.js
  • Update mlpm.js
  • Update responsive_menus_codrops_responsive_multi.js
  • Update responsive_menus_sidr.js

User interface changes

No

API changes

No

Data model changes

No

📌 Task
Status

Fixed

Version

1.0

Component

Code

Created by

🇧🇾Belarus gun_dose

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @gun_dose
  • Assigned to PrabuEla
  • Issue was unassigned.
  • Status changed to Needs review over 1 year ago
  • Assigned to Jay Jangid
  • Issue was unassigned.
  • 🇮🇳India Jay Jangid

    @PrabuEla I have tested this issue in Drupal 10 by installing this module and applying your patch.
    After the patch was applied I got this error:-

    The website encountered an unexpected error. Please try again later.
    
    Drupal\Component\Plugin\Exception\PluginNotFoundException: The "" plugin does not exist. Valid plugin IDs for Drupal\responsive_menus\ResponsiveMenusPluginManager are: google_nexus, codrops_responsive_multi, mean_menu, mlpm, responsive_menus_simple, sidr in Drupal\Core\Plugin\DefaultPluginManager->doGetDefinition() (line 53 of core\lib\Drupal\Component\Plugin\Discovery\DiscoveryTrait.php).

    Please look into this.

    Please guide me as if I am missing any steps to reproduce this issue.

    Here are the steps I took to reproduce the issue
    1. Install Responsive Menus module
    2. Check the Responsiveness of menus in home page.
    Thank you.

  • 🇨🇾Cyprus ddd200

    Please find the patches for Simple Expanding menu style. Tested on D10.1.2.

  • 🇨🇾Cyprus ddd200

    Please skip the responsive_menus_simple.js_.patch in comment #7. Find a correct one.

  • 🇮🇳India shubham_jain

    I also reproduced the steps and instead of multiple patches you can use this single patch.

  • Status changed to Needs work about 1 year ago
  • 🇬🇧United Kingdom rivimey

    The patch looks sane, but @shubham_jain please Do NOT post a single patch to multiple issues - it just wastes everyone's time thinking there are several different issues, and should an error or improvement be found in one issue's patch the other issues probably won't be kept in sync.

    The correct action is to Close(Duplicate) all but one of the issues, with a comment indicating the remaining issue link and in that remaining issue add your patch. Anybody can close (or open) an issue - it doesn't require the maintainer to do so.

    Also, I note that you have included in this patch the change to the core version requirement. It is generally a good idea to stick to fixing one issue in each patch, but moreover, the change just adds D10, while I would expect that the current module is no longer compatible with D8 (especially because of the need for 'once'. When modifying shared codebases it is important to do the whole job, not just the part that lets you move on.

    Marking Needs Work because of the core_version_requirement change needed.

  • 🇮🇳India shubham_jain

    Sorry @rivimey, when I uploaded this patch and other patches which were just of the same issue. I wasn't aware of the duplicate or checking that this will may cause codebase issue.

    I was new to the drupal org and that's why I thought that the maintainer can only do that. Or you can create issue fork and many other things.

    So, please let go of my mistake and sorry again.

  • 🇬🇧United Kingdom rivimey

    @shubham_jain we all make mistakes, so no problem. I explained at length to enable you to understand, that is all.

    I would encourage you to fill out your d.o profile a little more, but of course that is your choice.

  • 🇮🇳India shubham_jain

    Hi @rivimey, can you tell me what to do now or you close this issue.

  • 🇬🇧United Kingdom rivimey

    @shubham_jain the next thing to do is:

    Marking Needs Work because of the core_version_requirement change needed (remove it in favour of #3364838: Need to be D10 compatible)

    If you could post another version of the change that excludes changes to the .info.yml file, then I can apply it.

  • 🇮🇳India shubham_jain

    Hi @rivimey, the patch I uploaded in the comment #9 does not contains info.yml file.

    It only contains libraries.yml and responsive_menus_simple.js files changes. So you can apply it.

  • 🇳🇱Netherlands megachriz

    I see that there are more JS files that are using .once in the code, so I think that besides "responsive_menus_simple.js" that more JS files need to be patched. I think that these files also need to be updated:

    • responsive_menus_google_nexus.js
    • responsive_menus_mean_menu.js
    • mlpm.js
    • responsive_menus_codrops_responsive_multi.js
    • responsive_menus_sidr.js

    I also listed these in the issue summary .

  • 🇮🇳India shubham_jain

    Ok, I will look into those files as well.

  • 🇮🇳India shubham_jain

    Hi everyone, I have updated the all the js and libraries.yml to solve the issue.
    Please review and verify.

  • Status changed to Needs review about 1 year ago
  • 🇬🇧United Kingdom rivimey

    @shubham_jain the -18 patch looks reasonable, thanks.

    @MegaChriz would you be able to test it at all?

  • 🇺🇸United States jwjoshuawalker San Diego

    Does anyone here know how to add the patch / pull request in Drupal's GitLab so that the merge can be done here?

    I am away from the office for a while and there's a few issues for this project with patches posted, it is much easier to use that new system.

  • 🇮🇳India shubham_jain

    Hi @jwjoshuawalker, I can only create issue fork and push the changes to the issue branch. Creating a PR raise for the issue.
    But only the maintainer can merge those changes into the main branch.

  • 🇺🇦Ukraine goodboy Kharkiv, Ukraine

    The patch completely broke the script. Do you think this is the right replacement?

    -          $(iteration.selectors).once('responsive-menus-codrops-multi-menu', function() {
    +          $(once('responsive-menus-codrops-multi-menu', 'iteration.selectors'), function() {
    
  • Status changed to Needs work 9 months ago
  • 🇺🇦Ukraine goodboy Kharkiv, Ukraine
  • 🇮🇳India dineshkumarbollu

    Has my understanding the code

    +          $(once('responsive-menus-codrops-multi-menu', 'iteration.selectors'), function() {
    

    is incorrect format.

    it will replace like

    $(once('responsive-menus-codrops-multi-menu', iteration.selectors)).each(function(){

    @goodboy any thoughts on this?

  • 🇺🇦Ukraine goodboy Kharkiv, Ukraine

    @dineshkumarbollu, exactly

  • Assigned to dineshkumarbollu
  • 🇮🇳India dineshkumarbollu

    Will provide patch for all changes soon.

  • Issue was unassigned.
  • Status changed to Needs review 9 months ago
  • Status changed to RTBC 9 months ago
  • 🇮🇳India mukesh-kumar3 Dharmshala

    Hi @dineshkumarbollu i have applied the patch provided by you and is nicely added. and also it works in drupal 10. so moving this issue to RTBC

  • Status changed to Fixed 9 months ago
  • 🇺🇸United States jwjoshuawalker San Diego

    Pushed!

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

Production build 0.71.5 2024