Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia created an issue. See original summary.

larowlan’s picture

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.

droplet’s picture

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

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

cilefen’s picture

Priority: Normal » Major

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

larowlan’s picture

Title: With jQuery3, tours lose their grey background and centering » With jQuery3, modal tour tips lose their grey background and centering
davidhernandez’s picture

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.

cilefen’s picture

Issue tags: +rc target

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.

effulgentsia’s picture

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

Since this is a regression caused by the newly committed jQuery3, adding that to the issue title prefix.

larowlan’s picture

larowlan’s picture

Status: Active » Needs review
FileSize
26.17 KB

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.

droplet’s picture

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

Any reason that we must use upstream version?

larowlan’s picture

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

Status: Needs review » Needs work

The last submitted patch, 11: 2898808-tour-breakage.1.patch, failed testing. View results

cilefen’s picture

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

larowlan’s picture

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

larowlan’s picture

Status: Needs work » Needs review
FileSize
43.32 KB

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?

larowlan’s picture

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

larowlan’s picture

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

Manuel Garcia’s picture

@larowlan++

xjm’s picture

Issue tags: +8.4.0 release notes

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

Good call on the upstream PR.

GrandmaGlassesRopeMan’s picture

FileSize
1.32 MB
2.83 MB

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

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

droplet’s picture

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.

Eric_A’s picture

Version: 8.5.x-dev » 8.4.x-dev

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?

effulgentsia’s picture

Priority: Major » Critical

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

Wim Leers’s picture

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

effulgentsia’s picture

Status: Needs review » Needs work

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

GrandmaGlassesRopeMan’s picture

Status: Needs work » Needs review
FileSize
60.87 KB
43.95 KB

- 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

effulgentsia’s picture

Thanks!

  1. +++ b/core/core.libraries.yml
    @@ -397,7 +397,7 @@ jquery.joyride:
         url: https://github.com/zurb/joyride/blob/v2.1.0/README.markdown
         gpl-compatible: true
       js:
    -    assets/vendor/jquery-joyride/jquery.joyride-2.1.min.js: { minified: true }
    +    misc/jquery.joyride-2.1.js: { }
    

    Above this is a line that says:

    version: "v2.1.0"
    

    Is there a best practice way of identifying a version number for a fork? Or should we leave it as 2.1.0?

  2. +++ b/core/misc/jquery.joyride-2.1.js
    @@ -0,0 +1,928 @@
    +* jQuery Foundation Joyride Plugin 2.1
    +* http://foundation.zurb.com
    +* Copyright 2013, ZURB
    +* Free to use under the MIT license.
    +* http://www.opensource.org/licenses/mit-license.php
    

    I think a comment here linking to the pull request in #19, and maybe also to this d.o. issue, would be good.

  3. Can anyone think of anything else we should do to help people understand that this is a (hopefully temporary) fork?
effulgentsia’s picture

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

larowlan’s picture

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.

andypost’s picture

droplet’s picture

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

effulgentsia’s picture

Re #33, that's the result of a git describe on a clone of https://github.com/zurb/joyride at commit c2b38668, so I think is the correct version. Or should we use something other than the git describe?

droplet’s picture

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

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

Status: Needs review » Needs work

The last submitted patch, 31: 2898808-tour-bump.31.patch, failed testing. View results

effulgentsia’s picture

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

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

droplet’s picture

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’s picture

there's no caching issue

There 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 update LibraryDiscoveryParser if we want to pass that along to the js/css items, as we do for 'version'.

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

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

droplet’s picture

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

effulgentsia’s picture

We also used `commit` before.

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

the URL changed from min.js to .js. While it will refresh the cache

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.

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

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

And if you loading the CDN

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

effulgentsia’s picture

Status: Needs work » Needs review

Back 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 a commit key in libraries.yml files.

effulgentsia’s picture

Meanwhile, ticking the credit box for @droplet for all the reviews.

effulgentsia’s picture

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

Well, 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:

var version = "3.2.2-pre d2a380759f17abd54bb7576c6a1b40e2f069c3f3",

So, how about we match that? Here's a patch that does.

droplet’s picture

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 ^_^)

effulgentsia’s picture

For example: c4b192de5b5f569b00050125482d7c0c98c56b91

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

effulgentsia’s picture

And 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 and LibraryDiscoveryParser, but given the beta deadline, I'm hoping we don't need to block this issue on that coverage.

effulgentsia’s picture

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

cilefen’s picture

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.

droplet’s picture

Status: Needs review » Reviewed & tested by the community

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

cilefen’s picture

Which patch are you RTBCing?

droplet’s picture

Patch #47.

cilefen’s picture

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

droplet’s picture

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.

effulgentsia’s picture

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

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

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review
cilefen’s picture

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

GrandmaGlassesRopeMan’s picture

- Adjust version to 2.1.0.1

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/assets/vendor/jquery-joyride/jquery.joyride-2.1.js
+      'version'              : '2.1',
...
+++ b/core/core.libraries.yml
+  version: "2.1.0.1"

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

cilefen’s picture

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

  • cilefen committed 7b7779f on 8.5.x
    Issue #2898808 by effulgentsia, larowlan, drpal, droplet, cilefen: [...

  • cilefen committed 4066acc on 8.4.x
    Issue #2898808 by effulgentsia, larowlan, drpal, droplet, cilefen: [...
cilefen’s picture

Status: Reviewed & tested by the community » Fixed

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.

Status: Fixed » Closed (fixed)

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