Problem/Motivation
D10 will remove jQuery dependency. Any progress to remove this from extlink Module?
Steps to reproduce
Proposed resolution
Replace jQuery code with JavaScript
Remaining tasks
Test/review and add it to a new version of the module
User interface changes
none
API changes
none
Data model changes
none
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | extlink-after.png | 3.98 KB | tyler36 |
| #4 | 3238995_extlink_remove-jquery-dependency.patch | 13.97 KB | granik |
Issue fork extlink-3238995
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:
- 3238995-remove-jquery-dependency
changes, plain diff MR !5
Comments
Comment #4
granikCreated a merge request. The jquery code has been replaced with vanilla javascript. Checked on Firefox, Chrome and Safari, should work. But I would ask for review, it should be tested properly.
Also added a patch file to use it with composer.
Comment #5
grienauerComment #6
tyler36 commentedTested on Drupal `10.0.3`
## Test
1. Added internal-only Link field to 'Article' node.
2. Added external-only Link field to 'Article' node.
3. Created new article with internal & external links.
## Other
https://www.drupal.org/project/extlink will also need updating to remove the `jQuery` reference from the first paragraph.
Comment #7
solideogloria commentedComment #8
anybodyThanks! Conflicts need to be resolved!
Comment #9
granik@Anybody, thanks. Just rebased, but would be ok if you test your new mailto feature again after my rebase.
Comment #10
anybodyComment #11
granikComment #12
smustgrave commentedComment #13
smustgrave commentedWould like to include with 2.0.x but appears to breaking a number of tests.
Comment #14
smustgrave commentedWoo got main pipeline all green so any change should be legit issues.
Comment #15
solideogloria commentedThe rebase had lots of commits, because I merged once instead of rebasing prior to that.
Comment #16
solideogloria commentedI fixed almost all of the eslint issues. There are a few that I don't know if they should be ignored or changed.
Comment #17
smustgrave commentedWouldn't worry about those, but the test failures could be showing an issue.
Comment #18
solideogloria commentedBoth failing tests are because it's not finding the external settings file.
It has
Should it be this instead, with
/extlink/settings.js?See in extlink.module:
Comment #19
smustgrave commentedAh that took me forever to figure out yesterday. So the test may need to be updated to check the host. Gitlab running with localhost/web was causing all tests to fail before.
Comment #20
solideogloria commentedI think this will fix the tests. The functionality with the external settings is actually working when I run it. It's just checking for the wrong string in the response.
Comment #21
solideogloria commentedComment #22
smustgrave commentedReverted the changes to the test as don't think we should update those. If the jquery removal is correct then nothing should break. So think maybe the js changes around the nofollow/referrer needs some work.
Comment #23
smustgrave commentedDon't have the why yet but appears to be when the link has a target already set that the js fails to add a rel attribute
Comment #24
smustgrave commentedSo I moved the filter part of the js down after the rel is added. Wasn't needed before but tests appear green. Thoughts?
Comment #25
solideogloria commentedIt's better, but I don't think it's quite right. The
externalLinksvariable is filtered down, meaning that later whennoreferreris added, it won't be added to links that already had thetargetattribute set, even if thetargetis_blank. Is that the desired functionality? The form setting says this:My understanding of the "no override" setting was that it shouldn't override the
targetattribute, but that thenofollowandnoreferrershould still be set on the link.Personally, I think this
Should be modified so that
externalLinksis not changed. Something like this:Comment #26
smustgrave commentedApplied suggestion but have to run. If it works and we are happy I can merge this evening.
Comment #27
smustgrave commentedTests passed
Comment #28
solideogloria commentedLooks good to me. Do we need to add a test to cover that
noreferreris added to links with a target attribute already set?Comment #31
smustgrave commentedThink we are probably good.
Down to 2 issue so will plan a beta1 release in the next few days.