[regression] With jQuery3, modal tour tips lose their grey background and centering

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

I 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

I'd say this is at least major for tours.

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

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

Ok, 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

• update tour.module.css (in tour and seven and stable) so it doesn't look butt ugly - should be doable for someone with front-end skills
• work out what event is fired when the tour is complete so we can do the changes to the backbone model (commented out for now)
• work out how to add the close/progress elements to the template (commented out too)
• map the old data-options to the new data-{x} attributes - have done this already for data-class and data-id so its just more of the same - we will need to add some deprecation notices there as this is a BC shim - should be able to trigger_error in ->getAttributes method on TipPluginBase
• switch back to the .min.js (both included in this patch for debugging sake)

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.

v3.0 has 23KB addition (min.js). Solo version bundled with Foundation Core inside.

Any reason that we must use upstream version?

Yeah by all means lets ditch them, the foundation footprint adds a lot of cruft

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

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

This 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?

Obviously if we fork, we'd take it out of vendor and turn off minified flag in libraries.yml

Pull request to zurb/joyride https://github.com/zurb/joyride/pull/228

@larowlan++

We need to mention this in the release notes while it's still outstanding.

Good call on the upstream PR.

Tested #17 against the tour at /admin/config/regional/language,

and /admin/structure/views/view/content,

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

From the parent:

Also bumping to critical since if we don't get this into 8.4.x we'll be shipping stable 8.x releases with an unsupported jQuery version for 6+ months.

And then this:

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

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?

I 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

- moves core/assets/vendor/jquery-joyride/jquery.joyride-2.1.min.js to core/misc/jquery.joyride-2.1.js
- adjust core/core.libraries.yml

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

+++ b/core/core.libraries.yml
@@ -391,13 +391,13 @@ jquery.form:
+  version: "v2.1-11-gc2b3866"

wrong version I think

other than that, looks good.

shouldn't it use a sub-key `commit`

jquery.joyride:
  version: v2.1
  commit: c2b38668

we remove the min, there's no caching issue.

If we changed the version number, it doesn't match the script version:

+++ b/core/assets/vendor/jquery-joyride/jquery.joyride-2.1.js
@@ -0,0 +1,928 @@
+      'version'              : '2.1',

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

Sorry if this is a stupid question, but why does that matter?

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.

v2.1-11-gc2b3866

this is a new creative thing from this issue. (even append g before commit hash) doesn't it?

When have we used it in a .libraries.yml file?

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

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.

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.

git describe, which includes the "g" prefix, is not a Drupal-invented thing.

Oh thanks, didn't know that. Sorry.

Are untagged commits of JS libraries ever released to a CDN?

who knows what crazy things people will do

(I wrote it before comment #44 ^_^)

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

** revert jQuery 3.0 may break OutSide_In module. **

Which patch are you RTBCing?

Patch #47.

IMO #31 is better than an untested change to JsCollectionRenderer.

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

version_compare('2.1', '2.1.0') = -1 which is going to confuse anybody checking versions while altering libraries.

- Adjust version to 2.1.0.1

I am crediting myself for #57 and for spending time weighing options.

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