Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
After updating to jQuery 3 via the patch in #2533498-79: Update jQuery to version 3, tours:
- No longer grey out the underlying page
- No longer start out centered
Before jQuery3:
After jQuery3:
Comment | File | Size | Author |
---|---|---|---|
#58 | 2898808-58.patch | 44.68 KB | GrandmaGlassesRopeMan |
#58 | interdiff-2898808-55-58.txt | 583 bytes | GrandmaGlassesRopeMan |
#55 | interdiff-31-55.txt | 533 bytes | effulgentsia |
#55 | 2898808-tour-bump.55.patch | 44.54 KB | effulgentsia |
#47 | interdiff-44-47.txt | 2.28 KB | effulgentsia |
Comments
Comment #2
larowlanThere are two types of tour tips - those that are element specific, and the modals.
The specific ones work, the modal ones do not.
I suspect this will be an upstream issue and may be resolved by updating our version of joyride.
Comment #3
droplet CreditAttribution: droplet commentedI don't want to introduce jquery.migrate for tour.js because Tour module enabled by default.
I think there's 2 directions:
1. Fork it and add features we wanted. The upstream move to Foundation based.
2. Replace it with another libs.
(3). Move it to jQuery UI Dialog
Comment #5
cilefen CreditAttribution: cilefen commentedI'd say this is at least major for tours.
Comment #6
larowlanComment #7
davidhernandezYeah, major probably. Does its positioning still work correctly when stepping through so that its arrows are pointing to the right places? If not, than this is critical.
Comment #8
cilefen CreditAttribution: cilefen commentedIt does. However, the initial modal thing is so far down the page you think tours is broken. So for me this is critical or an rc target.
Comment #9
effulgentsia CreditAttribution: effulgentsia at Acquia commentedSince this is a regression caused by the newly committed jQuery3, adding that to the issue title prefix.
Comment #10
larowlanComment #11
larowlanOk, this updates to latest standalone (solo) zurb joyride plugin from https://github.com/zurb/joyride/blob/v3.0/dist
Added todos in code for remaining work but in summary
long story short, it is working but ugly - but I think should be simple for someone with better css chops than me to resolve fairly easily.
Comment #12
droplet CreditAttribution: droplet commentedv3.0 has 23KB addition (min.js). Solo version bundled with Foundation Core inside.
Any reason that we must use upstream version?
Comment #13
larowlanYeah by all means lets ditch them, the foundation footprint adds a lot of cruft
Comment #15
cilefen CreditAttribution: cilefen commented@xjm and I are concerned by #8. We have decided to revert #2533498: Update jQuery to version 3 if this issue isn't fixed before 8.4.0-beta1. We don't want to discourage work on this—quite the opposite! So, keep at it.
I am cross-posting this.
Comment #16
larowlan@droplet would you prefer we fork the 2.1.0 version and make it jQuery 3 compatible?
https://github.com/zurb/joyride/pull/219/files is a WIP towards that, although it looks to be adding some new features too.
Stuff like https://github.com/zurb/joyride/pull/219/files#diff-528765f2eeb4f69973c2... we obviously need.
And I think https://github.com/zurb/joyride/pull/219/files#diff-528765f2eeb4f69973c2... (all flavours of that diff) is the reason for the regression.
Comment #17
larowlanThis makes the changes to joyride as per #16 - and it works.
Do we have any contacts to enquire if this would be welcome for the 2.1 branch?
Comment #18
larowlanObviously if we fork, we'd take it out of vendor and turn off minified flag in libraries.yml
Comment #19
larowlanPull request to zurb/joyride https://github.com/zurb/joyride/pull/228
Comment #20
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commented@larowlan++
Comment #21
xjmWe need to mention this in the release notes while it's still outstanding.
Good call on the upstream PR.
Comment #22
GrandmaGlassesRopeManTested #17 against the tour at
/admin/config/regional/language
,and
/admin/structure/views/view/content
,Comment #23
droplet CreditAttribution: droplet commentedAha!!! didn't expect that little changes. @larowlan++!!!
https://jquery.com/upgrade-guide/3.0/#breaking-change-deprecated-context...
this is the first time I know jQuery 2.x and below have these 2 properties. LOL.
I'm fine with this patch get in.
Comment #24
Eric_A CreditAttribution: Eric_A commentedFrom the parent:
And then this:
Should this follow-up have the same priority as #2533498: Update jQuery to version 3? Or is the the jQuery3 update no longer considered a critical 8.4.x task?
Comment #25
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI agree with #24 that this should now be Critical for the same reason #2533498: Update jQuery to version 3 was. I guess we're still waiting to hear back on https://github.com/zurb/joyride/pull/228#issuecomment-320534479, since it would be best to not have to fork joyride.
Comment #26
Wim LeersI pinged the maintainer on Twitter, since @larowlan last pinged him on the GitHub issue 3 days ago, and there's still no response. The last time we heard from the maintainer is now 7 days ago.
https://twitter.com/wimleers/status/895270110180634624
Comment #27
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThere's less than 24 hours till the beta commit freeze. I think it's time to fork the joyride library. We can still unfork it later if/when it gets fixed upstream. Needs work per #18.
Comment #28
GrandmaGlassesRopeMan- moves
core/assets/vendor/jquery-joyride/jquery.joyride-2.1.min.js
tocore/misc/jquery.joyride-2.1.js
- adjust
core/core.libraries.yml
Comment #29
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThanks!
Above this is a line that says:
Is there a best practice way of identifying a version number for a fork? Or should we leave it as 2.1.0?
I think a comment here linking to the pull request in #19, and maybe also to this d.o. issue, would be good.
Comment #30
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThe PR was committed an hour ago! https://github.com/zurb/joyride/pull/228#issuecomment-322262629
Not sure yet if we'll get a tagged release (2.1.1) today or not.
Comment #31
larowlanHere's a patch that updates to v2.1-11-gc2b3866 which is the fix we need.
Note they don't provide a .min version anymore, so I updated filename and libraries conf.
Comment #32
andypostComment #33
droplet CreditAttribution: droplet commentedwrong version I think
other than that, looks good.
Comment #34
effulgentsia CreditAttribution: effulgentsia at Acquia commentedRe #33, that's the result of a
git describe
on a clone of https://github.com/zurb/joyride at commitc2b38668
, so I think is the correct version. Or should we use something other than thegit describe
?Comment #35
droplet CreditAttribution: droplet commentedshouldn't it use a sub-key `commit`
Comment #37
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI don't think we have a precedent for that in any of our *.libraries.yml files. When JS aggregation is disabled, the version is appended to the query string (by
JsCollectionRenderer
), but we do not yet append a commit ID to that query string. Doing so might be a good followup though.Comment #38
droplet CreditAttribution: droplet commentedwe remove the min, there's no caching issue.
If we changed the version number, it doesn't match the script version:
Comment #39
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThere is. In
JsCollectionOptimizer::generateHash()
, the value of a library's 'commit' key isn't part of the data from which a hash is generated. We would need to updateLibraryDiscoveryParser
if we want to pass that along to the js/css items, as we do for 'version'.Sorry if this is a stupid question, but why does that matter?
Comment #40
droplet CreditAttribution: droplet commented@effulgentsia,
In this case, the URL changed from min.js to .js. While it will refresh the cache, I don't see any reason we need to create an unused thing from this issue. We also used `commit` before.
Yeah, the same version string of script and YML is less important but that's what we should do when we can.
And if you loading the CDN or verify it, you will compare the version number.
this is a new creative thing from this issue. (even append g before commit hash) doesn't it?
Comment #41
effulgentsia CreditAttribution: effulgentsia at Acquia commentedWhen have we used it in a
.libraries.yml
file? I like the concept, but AFAIK, it's new and we'd need to add support for it per #39.This time it will because of the filename change. But if in the future we need to change the commit to another commit, and changed nothing else, then it won't refresh the cache.
git describe, which includes the "g" prefix, is not a Drupal-invented thing.
Are untagged commits of JS libraries ever released to a CDN?
Comment #42
effulgentsia CreditAttribution: effulgentsia at Acquia commentedBack to Needs Review, since the failure in #36 was when HEAD was broken, and #31 is now green again.
Per #33, I think the only thing keeping this from RTBC at this point is the conversation since then about figuring out if the version string generated from
git describe
is acceptable, or if we need to add support for acommit
key in libraries.yml files.Comment #43
effulgentsia CreditAttribution: effulgentsia at Acquia commentedMeanwhile, ticking the credit box for @droplet for all the reviews.
Comment #44
effulgentsia CreditAttribution: effulgentsia at Acquia commentedWell, yes. E.g., the ones at the top of https://code.jquery.com/jquery/ listed under "UNSTABLE, NOT FOR PRODUCTION".
And in https://code.jquery.com/jquery-git.js, they use this pattern:
So, how about we match that? Here's a patch that does.
Comment #45
droplet CreditAttribution: droplet commentedFor example: c4b192de5b5f569b00050125482d7c0c98c56b91
I'm sure it's not the only usage. Someone rejected my CORE patch with same reason before. that's why I know it.
Let's focus on this issue first. We introduced a new standard without a discussion and we rushing to beta deadline. It's not good.
Oh thanks, didn't know that. Sorry.
who knows what crazy things people will do
(I wrote it before comment #44 ^_^)
Comment #46
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI tracked this one down. Looks like the patch that silently introduced a 'commit' key within a libraries.yml file was #1996238-60: Replace hook_library_info() by *.libraries.yml file, with no discussion on the issue about it, and no mention of it in that issue's change record. It got later silently removed as a side-effect of upgrading CKEditor to a tagged release in #2039163: Update CKEditor library to 4.4.
Both the insertion of it and removal of it were done during Drupal 8.0's alpha phase. It doesn't appear to have ever come up (that I can find) since 8.0.0's release, and I don't think we have any docs anywhere mentioning it.
Comment #47
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAnd here's a way for doing it the way suggested in #35, but solving for #39.
If we choose this approach, we'd need to add test coverage for the changes to
JsCollectionOptimizer
andLibraryDiscoveryParser
, but given the beta deadline, I'm hoping we don't need to block this issue on that coverage.Comment #48
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI don't have a strong preference between #31, #44, and #47. I think any one of them is preferable to not committing the joyride fix and instead shipping a beta with a broken tour module or reverting jQuery3 to jQuery2.
My weak preference is to go with #31, since that doesn't require any changes to
core/lib
. And to have a post-beta followup for deciding on whether and when to switch to one of the other approaches.Comment #49
cilefen CreditAttribution: cilefen commentedWe are resolved to revert jQuery 3 if this is not solved rather than break a module in beta1 so the simplest possible solution right now will be the best one.
Comment #50
droplet CreditAttribution: droplet commented** revert jQuery 3.0 may break OutSide_In module. **
Comment #51
cilefen CreditAttribution: cilefen commentedWhich patch are you RTBCing?
Comment #52
droplet CreditAttribution: droplet commentedPatch #47.
Comment #53
cilefen CreditAttribution: cilefen commentedIMO #31 is better than an untested change to JsCollectionRenderer.
Comment #54
droplet CreditAttribution: droplet commentedNothing better than hit bug in Beta than GA version.
There are about 2 months to release. If we don't take the risk and think we will fail to add a test or commit hash or fix it, just keep the v2.1 IMO.
there's not every repo following semantic versioning. #31 syntax is unpredictable in many cases.
Contrib modules always copying the CORE style, it will not a single exception in CDN-like modules.
Comment #55
effulgentsia CreditAttribution: effulgentsia at Acquia commentedSo, turns out we already have a pre-existing condition in HEAD of a potentially non-unique library version:
jquery.farbtastic
has a version of just "1.2".Therefore, how about we allow ourselves to continue that pattern here, and have a follow-up for either adding a "commit" key (e.g., #47 plus test coverage) or defining some other pattern for making version unique (e.g., #31, #44, or something else)?
Here's a patch that does that. Interdiff is relative to #31.
Comment #56
effulgentsia CreditAttribution: effulgentsia at Acquia commentedComment #57
cilefen CreditAttribution: cilefen commentedversion_compare('2.1', '2.1.0') = -1 which is going to confuse anybody checking versions while altering libraries.
Comment #58
GrandmaGlassesRopeMan- Adjust version to
2.1.0.1
Comment #59
effulgentsia CreditAttribution: effulgentsia at Acquia commentedIt's a shame for these not to match. It would be nice if we could trust the vendor's version. But I think #57 is the more important consideration, so RTBC.
Comment #60
cilefen CreditAttribution: cilefen commentedI am crediting myself for #57 and for spending time weighing options.
Comment #63
cilefen CreditAttribution: cilefen commentedCommitted 7b7779f and pushed to 8.5.x and cherry-picked to 8.4.x as 4066acc. Thank you everyone.
Let's consider a follow-up to enhance the library parsing so we have a documented way to handle situations like this.