De-fork jQuery Joyride and update to latest stable release

Clemens,

You should run the version of joyride that is provided via core. That is trialed and tested in other issues.

I believe that we should fork joyride so we can add our "Previous" button to it. I have had an issue open for quite some time without response:

https://github.com/zurb/joyride/pull/84

I will ask around and find out what can be done.

For now I suggest you test with the version of joyride that is committed to core.

@clemens
I already tried version 2.1 and it worked for me, I didn't ran into any issue while using it. Only adding autoStart: true to tour.js is needed.

But in the end I think it is better to build our own jQuery tour plugin, (based on the joyride plugin).
Thereby the Drupal community gets the opportunity to implement features itself.

Add ?tour=1 to the URL for auto start

@lucastockmann you're volunteering for maintainer of tour module then?

@larowlan Adding ?tour=1 to the URL only initialize the joyride object. In version 2.1 of the joyride plugin it needs to call the show method to show a tip. While setting autoStart: true at creating the joyride object, the show method gets called automatically.

@nod_ I think someone with more experiences than me should maintain the tour module. But I'm willing to help as far as I can!

The latest release has built-in support for ARIA and many other gems.

We should de-fork this library and replace it with a copy of an upstream release.

I agree. I believe those ARIA roles where committed upstream as a PR from @larowlan. Ill take the latest for a test run.

This should be a new ticket, but should we look at something like https://github.com/bower/bower to ensure we are running stables releases? I might take a look at this at the same time.

Bower is a larger topic, discussed as part of #1663622: Change directory structure for JavaScript files

For now, we just have the new library declarations, which at least allow us to manually script a version/update check, as performed in the parent meta issue.

That's very true, thanks for pointing me at that issue sun!

Adjusting issue title accordingly.

I am attaching a patch with update to 2.1 to see if any tests fail. The funny thing is that the js file says it is version 2.0.3. This will need manual testing too.

I dug around a little bit and see that the versions were fixed in later commits but not tagged. I am wondering if we should take them. If so, then we could as well take the latest HEAD (committed in Jan 2014). Thoughts?

Oops... Just a small change in core.libraries.yml. The filename is jquery.joyride-2.1.js and I used jquery.joyride-2.1.0.js. I directly edited the patch file, so no interdiff.

@see: #2203431: [meta] Various asset (JavaScript) libraries have to be updated to a (minified) stable release prior to 8.0.0

@droplet: I came here from that issue but I can't figure out what you mean. Am I missing something that needs to be done here?

@hussainweb,

at this stage, we needed to ship minified version of all 3rd party JS libs. also see #1341792: [meta] Ship minified versions of external JavaScript libraries

Thanks.

Okay. I kind of read it as "if available", but there is no minified version of this library available. Should we create one ourselves?

Yes. we should. We did it before: https://www.drupal.org/node/2207629#comment-9443237

Replaced jquery.joyride-2.1.js in the patch from comment #16 with a minified version of it (created with "uglifyjs"), and updated core.libraries.yml to match. I then manually edited the informational comment at the top of the minified version to decrease its size, and changed the version to 2.1 in the comment only--I didn't touch the incorrect version "2.0.3" coded further down. This incorrect version number exists in the official release tagged as "2.1" and "2.1.0" (see: https://github.com/zurb/joyride/blob/v2.1.0/jquery.joyride-2.1.js )

It also appears that maintenance of jQuery Joyride has been moved into zurb/foundation: https://github.com/zurb/foundation

There has been much activity on that project since jQuery Joyride's 2.1 release, which is about 18 month old now. Do we need to take that into account and try to pull the latest version out of zurb/foundation? Or, is that not required since it is now a component of a different software package?

The interdiff for my change in #22

Bumping to critical since the parent issue is.

If the maintained version is zurb/foundation we should at least open a new issue to look at moving over to that.

I can review the functionality of this one. I will have this done in the next few hours. However, one of the gates for this should be around Javascript, we will need a review for that as well.

Reasons for fork:

• https://www.drupal.org/node/1809352#comment-7035312 - a11y fixes (we sent PR upstream) - these might be in HEAD of joyride now with any luck
• https://www.drupal.org/node/1809352#comment-7040526 which lead to http://cgit.drupalcode.org/sandbox-larowlan-1790736/commit/core/modules/... which altered some global joyride invasive css - this one might be a problem

Not seeing any css changes in the patch - does that mean we're only de-forking the JavaScript?

tour doesn't show

Sorry Nick, didn't mean to take over

No, it's all good mate :)

Ill keep and eye on this ticket and review the joyride releases tonight to see how the library has tracked.

Initializing autoStart to true in /core/modules/tour/js/tour.js fixed the problem of the tour not showing when the Tour button was clicked.

It appears the joyride CSS had previously been incorporated into the Tour module in /core/modules/tour/css/tour.module.css, and in the Seven theme in /core/themes/seven/css/components/tour.theme.css. There were 4 additions to the official joyride CSS between joyride-2.0.3 and joyride-2.1., as follows:

.joyRideTipContent { display: none; }

.joyride-tip-guide span.joyride-nub.top-right {
  border-color: #000;
  border-color: rgba(0,0,0,0.8);
  border-top-color: transparent !important;
  border-left-color: transparent !important;
  border-right-color: transparent !important;
  top: -28px;
  bottom: none;
  left: auto;
  right: 28px;
}

.joyride-expose-wrapper {
    background-color: #ffffff;
    position: absolute;
    z-index: 102;
    -moz-box-shadow: 0px 0px 30px #ffffff;
    -webkit-box-shadow: 0px 0px 30px #ffffff;
    box-shadow: 0px 0px 30px #ffffff;
}

.joyride-expose-cover {
    background: transparent;
    position: absolute;
    z-index: 10000;
    top: 0px;
    left: 0px;
}

I've incorporated these additions into the Tour and Seven CSS files as best I could. It doesn't appear that the Tour module will currently make use of any of these additions.

In response to @larowlan in #27, I have verified that the a11y fixes are incorporated in the official joyride-2.1. The invasive CSS is not an issue due to the way in which the joyride CSS has been incorporated into Tour and Seven.

+++ b/core/modules/tour/js/tour.js
@@ -102,6 +102,7 @@
+        	autoStart: true,

there is some tab whitespace here, can you update to use spaces?

Confirm from manual testing that this is working!

Once you've fixed the spaces issue this is RTBC.

Whitespace fixed. Thanks @larowlan

looks good to me » over to nick for a review

1. +++ b/core/modules/tour/css/tour.module.css
   @@ -51,6 +51,10 @@
   +.joyRideTipContent {
   +  display: none;
   +}
   +
   

   To my understand. Tour is a custom Drupal module? .joyRideTipContent isn't match drupal code standard. And I couldn't find this class used in anywhere.

2. +++ b/core/modules/tour/css/tour.module.css
   @@ -145,3 +156,15 @@
   +  top: 0px;
   +  left: 0px;
   

   Could you drop 'px' when you do a patch reroll

3. +++ b/core/modules/tour/js/tour.js
   --- a/core/themes/seven/css/components/tour.theme.css
   +++ b/core/themes/seven/css/components/tour.theme.css
   

   Also, I wonder how many code from (https://github.com/zurb/joyride/blob/master/joyride-2.1.css) copied into tour.theme.css & modified. Shouldn't put it to core/assets/vendor/jquery-joyride ?

In response to @droplet in #39:

1. This selector was added to the official joyride-2.1.css release, so I incorporated it based on that. I can't find any reasoning given for the addition in the zurb GitHub repo, nor anything in the source code or the Tour module that would utilize it. It probably could be removed. In fact, none of the new CSS selectors from joyride-2.1 are being used by the current Tour module, from what I can tell.
2. Agreed
3. It appears the CSS component of joyride was incorporated into the Tour module since its creation, with some components moved into the Seven theme. Since then it has been heavily modified to be more consistent with Seven's look-and-feel. Based on that, I simply added the CSS selectors that were new in joyride-2.1 to the Tour module and Seven themes to support any new features/fixes in joyride-2.1. I'm not sure how much value there would be in incorporating https://github.com/zurb/joyride/blob/master/joyride-2.1.css into core/assets/ at this point.

I have reviewed and performed manual testing. If we can resolve the items in #40 I will be happy to RTBC.

I should add a screenshot.

I implemented @droplet's 1st and 2nd suggestions in #39. I let my reasoning in #40 stand for the 3rd suggestion. Manually tested to make certain that nothing broke.

If everyone is OK with this, I think we can go ahead and RTBC.

Happy with 40.3 - we have our own branding

Actually, I think that stylistic CSS more properly belongs in Seven theme, not Tour module. Or at least, screenshots of Tour module in Stark should be really, really boring. If they're highly branded like that, that's incorrect. However, it sounds like #40 is saying this is a pre-existing condition (and the diff seems to back that up), so it doesn't make sense to hold up a critical on moving around CSS. However, we should spin off a (normal) follow-up issue to look into this more and adjust the CSS as needed.

So with that out of the way...

Committed and pushed to 8.0.x. Great work, Core Critical Hours folks! :D

The joyride-related CSS contained in the Tour module deals with non-stylistic things like positioning, display, z-index, etc. Any stylistic CSS was placed in the Seven theme. I've verified that the Tour does in fact look very plain when using the Stark theme :) Not sure if creating a follow-up issue is necessary with this taken into account.

Opinions?

Awesome, that looks good to me. Thanks for checking.

Because of the way joyride has been incorporated into "zurb Foundation", it is not usable in isolation if pulled from the zurb Foundation repo. Therefore, one of these things would need to be done to use it from there:

1. Fork the latest joyride.js file again so we can make it work in isolation of zurb Foundation, or
2. Incorporate the entire zurb foundation package, which is quite large and complex. Probably not a good idea. It might be possible to extract just a few parts of Foundation to get what we need to have joyride work without modification, but that would be complex in a whole other way.

Alternatively, #1921136 could use joyride v2.1 (the version this issue put into core) as the basis for a fork.

Either way, since the intent of this issue was to de-fork joyride, it seems to me that further work should be done in other issues.

21 kb more for backend module may run single time only. I think it's fine.

https://github.com/zurb/foundation/blob/master/js/foundation/foundation.js

Any idea why do we pick joyride at first ?

still a lot of similar lib available , eg : http://usablica.github.io/intro.js/

I don't know why Joyride was chosen as the basis, but it was done quite a while ago and a lot has changed since then.

Noticing @catch's recommendation in #24, I've created a new issue pertaining to incorporating Joyride from zurb Foundation: #2409861: Explore replacing obsolete Joyride asset with latest version