Problem/Motivation

Reduce drupal learning curve and frustration.

Proposed resolution

write tour.module and add it to core (with an example in views)

Remaining tasks

  • reviews
  • followups

Steps to test

  1. git pull to get most recent drupal 8
  2. apply most recent patch
  3. install
  4. add a view and edit it

User interface changes

2013-02-18_1223.png

API changes

Needs to be described.

Follow-ups

Original report by @cweagans

I believe that this would help to solve #1164760: [meta] New users universally did NOT understand that you can extend Drupal.

Here's the idea:

At the end of the installer, users have to click the "Visit your new site" link. We should add another link to this page: something to the effect of "Learn how to use your new site". There would likely be very little difference between these links. "Learn how to use your new site" should link to the front page with "?tour=1" appended.

ZURB has a very handy jQuery plugin called Joyride that we can use for the user interface (MIT license): http://www.zurb.com/playground/jquery-joyride-feature-tour-plugin

This plugin also works with responsive layouts, which is definitely a plus: http://foundation.zurb.com/files/zurb-joyride-2/demo/demo.html

We should use this to point out different functionality provided by the current install profile (this should probably be described by a hook in the install profile, as these points will be different between Standard and, for instance, commerce_quickstart). This could be things like highlighting different links on the admin toolbar.

The bulk of tour.module will be a state machine, and I can't see it being too difficult to build.

We just need some way of targeting elements on a given page and displaying some text. Clicking next will move to the next element in the list. If it's the last element on the page, clicking next should redirect to the next page specified by the install profile hook.

I'm thinking about spending some time on writing this, and I'd love to work with somebody on it, but first, I think we should talk about it for a day or two in order to make sure that it's the right thing to do for core.

CommentFileSizeAuthor
#164 interdiff.txt468 bytesnick_schuch
#164 1809352-163.patch1.85 KBnick_schuch
#162 1809352-162.patch1.39 KBjthorson
#154 tour_module-1809352-154.patch14.79 KBnick_schuch
#148 tour-module-1809352.146.interdiff.txt3.43 KBlarowlan
#148 tour-module-1809352.146.patch76.34 KBlarowlan
#140 2013-02-18_1223.png130.36 KBYesCT
#137 interdiff_121-to-137.txt4.2 KBjessebeach
#137 tour_module-1809352-137.patch81.94 KBjessebeach
#134 2013-02-18_205925.png35.54 KBvijaycs85
#122 tour_module-1809352-121-interdiff.txt8.36 KBnick_schuch
#122 tour_module-1809352-121.patch78 KBnick_schuch
#119 tour_module-1809352-118.patch77.97 KBnick_schuch
#118 tour_module-1809352-118.interdiff.txt545 bytesnick_schuch
#117 tour_module-1809352-117-interdiff.txt1.84 KBnick_schuch
#117 tour_module-1809352-117.patch77.97 KBnick_schuch
#115 tour_module-1809352-115.patch79.14 KBWim Leers
#115 interdiff.txt11.61 KBWim Leers
#113 108-111-interdiff.txt1.39 KBjthorson
#111 tour-module-1809352.111.patch77.5 KBjthorson
#108 tour-module-1809352.108.interdiff.txt1.54 KBnick_schuch
#108 tour-module-1809352.108.patch77.5 KBnick_schuch
#104 tour-module-1809352.104.interdiff.txt130.39 KBnick_schuch
#104 tour-module-1809352.104.patch77.33 KBnick_schuch
#94 tour-module-1809352-94.patch80.53 KBWim Leers
#94 interdiff.txt17.85 KBWim Leers
#80 tour-module-1809352.80.patch73.84 KBlarowlan
#79 tour-module-1809352.79.interdiff.txt1.71 KBlarowlan
#79 tour-module-1809352.79.patch0 byteslarowlan
#76 Screen Shot 2013-02-12 at 5.55.40 PM.png29.4 KBlarowlan
#75 interdiff.txt4.08 KBnick_schuch
#75 tour-1809352-75.patch73.46 KBnick_schuch
#72 tour-1809352-72.patch72.65 KBtim.plunkett
#72 interdiff.txt25.8 KBtim.plunkett
#69 1809352-69-diff.txt3.22 KBnick_schuch
#69 1809352-69-tour-module.patch71.46 KBnick_schuch
#61 1809352-61-diff.txt28.98 KBnick_schuch
#61 1809352-61-tour-module.patch71.46 KBnick_schuch
#63 TourIcon.png1.55 KBCB
#63 tour-blu.png5.94 KBCB
#63 tour_icon12.jpg14.98 KBCB
#59 toolbar_look_questionmark.png3.02 KBtsvenson
#59 tour_step_below.png38.93 KBtsvenson
#59 tour_step_pointing_wrong.png10.62 KBtsvenson
#44 tour-module-1809352.44.patch70.81 KBeffulgentsia
#44 interdiff.txt8.48 KBeffulgentsia
#41 toolbar-tray-out-of-place.png44.62 KBjessebeach
#34 tour-module-1809352.34.patch67.68 KBlarowlan
#27 tour-module-1809352.27.interdiff.txt19.16 KBlarowlan
#27 tour-module-1809352.27.patch75.1 KBlarowlan
#20 1809352-tour-module.patch46.29 KBnick_schuch
#21 tour_example.png75.14 KBnick_schuch
#3 add-tour-module-1809352-3.patch33.16 KBDevin Carlson
#3 tour_link.png68.38 KBDevin Carlson
#3 tour_start.png77 KBDevin Carlson
#3 tour_step.png80.69 KBDevin Carlson
#3 tour_extension_information.png81.51 KBDevin Carlson
#3 tour_mobile_start.png38.34 KBDevin Carlson
#3 tour_mobile_step.png42.56 KBDevin Carlson
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

+1, we were talking about a slideshow while the installer ran at one point too, like you get when you install Ubuntu/Fedora

tim.plunkett’s picture

That joyride library is pretty awesome. +1

Devin Carlson’s picture

Status: Active » Needs review
FileSize
42.56 KB
38.34 KB
81.51 KB
80.69 KB
77 KB
68.38 KB
33.16 KB

I like the idea of having a better introduction to a new site than "Welcome to Site Name: No front page content has been created yet.".

It's not nearly as complex what cweagans described in the original issue, but I've attached an example tour module as a proof of concept. I've kept everything inside of the module folder and outlined the tour in a single template for simplicity's sake.

Tour module:

  • Adds Joyride as a library.
  • Assumes that you're only using the standard profile.
  • Adds a "take a tour" link to the bottom of the front page if you haven't created any nodes.
  • Walks the user through some of the default interface elements.

Installation

  • Install the standard profile.
  • Install tour module.
  • Visit the front page and click on the "take a tour" link.

Notes

  • Works best with the shortcut bar collapsed.
  • Is responsive and works pretty well at lower resolutions.
webchick’s picture

Issue tags: +Usability

There was some work done on this by Becky from Google at http://groups.drupal.org/node/223404 in the usability group which might be of interest. Also tagging.

Status: Needs review » Needs work

The last submitted patch, add-tour-module-1809352-3.patch, failed testing.

cweagans’s picture

Status: Needs work » Needs review

Wow, I didn't think that there'd be this much support, much less a patch! It seems that we're pretty much in agreement that this should happen.

@Devin, awesome work! I don't think we should add this to core unless 3rd party distros can hook in with their own tour steps, though. This is a very awesome start, but we should work up an API around it so that profiles can define their own tour steps and contrib modules can alter them. It shouldn't be too difficult to do, because at that point, we're just building a ul with the data from the hook implementations. We'll also need to make sure that it degrades gracefully...that might be difficult (having a gigantic ul on the page doesn't make a lot of sense).

Can we set up a time to talk on IRC or Skype and lay down a solid plan for exactly how this should work?

cweagans’s picture

Status: Needs review » Needs work

Whoops.

Grayside’s picture

Depending on the script, this could be useful to any new site member, and anyone promoted into broad administrative powers.

Worth thinking about not only hooks for an extensible tour, but also multiple tours. I can imagine contrib Rules integration.

yoroy’s picture

What a pleasant surprise this issue is! It seems like one important decision is wether this ought to be a contained slideshow like in http://onramp.lewisnyman.co.uk/ or the approach here, where we point out actual interface elements.

Fabianx’s picture

Status: Needs work » Needs review

#3: add-tour-module-1809352-3.patch queued for re-testing.

Fabianx’s picture

Huge +1 for this.

What is missing for RTBC besides a re-roll probably?

cweagans’s picture

I don't think this is flexible enough for core inclusion yet. To be useful outside of core, I think that we need to have a way to describe tour steps in an install profile, alter them in enabled modules, and allow for multi-page tours.

I think what Devin has written is a good start, but I think we need to make it useful for contrib profiles before adding it to core.

cweagans’s picture

I think we also need a different place to put the Start Tour link - I can imagine some cases where a user should be able to take a tour when there is existing content in the system (as noted above, the scenario where a user is promoted into new admin powers).

Bojhan’s picture

I have asked the people working on the tour in the usability group to chime in, this feels really exciting though. I feel like you should be able to start the tour on several places, e.g. also in the views interface.

@cweagans Could you perhaps help out on making it more flexible? In terms of messaging, we can actually use green messages for this? Or something similar, that is dismiss able, there are plenty of practices around this.

@Fabianx It would really help if you could stop calling for RTBC, prematurely. It confuses new contributors, and we all know this needs more thought and reviews.

jenlampton’s picture

For starters, I think this is a really, really great idea. +10!
However, I'm not sure it completely eliminates the need for an onramp like in http://onramp.lewisnyman.co.uk.

There is only so much we can do by pointing to things in the interface, people need to understand the big picture too, and before they get to the interface details. Drupal has content types, this is why you need one. Now, go here to add a new one. then Drupal has fields, this is why you need them. Go here to add a field. To new people, everything is a "page" and without understanding why they need a type, showing them a link to create new ones isn't going to help very much.

Is there a way we can have a screen or two in each section explaining the "what" and the "why" before we get to the "how" & the "where"?

cweagans’s picture

There is no requirement to have the tour steps point to interface elements. You can simply have a modal information panel as demonstrated in the patch above (see tour_start.png).

I am wary of putting a bunch of time into core these days. If I have time this Friday/Saturday/Sunday (after Thanksgiving), I will, perhaps, spend some time on this.

Fabianx’s picture

It would really help if you could stop calling for RTBC, prematurely. It confuses new contributors, and we all know this needs more thought and reviews.

I am sorry if this came across the wrong way, but this issue was essentially dead and is bound by feature freeze. I did not know that it needed more thoughts and reviews as no one had provided any since almost a month. (though I had mis-read cweagans feedback).

I essentially wanted to know what needs to happen to get this in. I'll be more specific next time. Okay? :-)

Fabianx’s picture

#12: Can you create a minimal plan to get this into core?

I would say:

* Get the library in
* Create a hook_tour_info with item
** machine name
** name
** description
** path (to show as base), maybe start: path?tour=
** tour items (array) - see below

* Each item would have:
** machine name (key)
** JS selector (optional)
** non JS description of selector for AX
** title
** description
** modal or not modal flag
** #weight

The JS might trigger an event like: trigger('tour-item-start', { item: "machine name"})

So that other JS can re-act to it and for example open a menu or such.

Further we IMHO need:

* Create permission (can view tour)
* A way to start the tour (append ?tour=1 to things?)
* Maybe an indicator that a tour is available (can be follow-up?)

That way we can:

* Define tours (hook_tour_info)
* Extend / Modify tours (hook_tour_info_alter)
* Extend tours (via JS)
* Run tours

Anything I am missing for a minimum viable product / module?

Thanks!

nick_schuch’s picture

Assigned: Unassigned » nick_schuch
Status: Needs review » Needs work

I have worked on a plugin solution over the past few days. I will post plans and details of this work very shortly.

nick_schuch’s picture

FileSize
46.29 KB

The goal for tour.module is to replace hook_help implementations with user interactive tour functionality powered by plugins and yaml files.

I have attached a patch of my current work. It provides:
- Implementation of hook_library_info. (Implements joyride library).
- Defined a new permission.
- Plugin to determine tour items.
- Toolbar item to toggle tour on/off.

A screencast of this tour.module in action:
http://www.youtube.com/watch?v=GlAQyuo1NsU

Some screenshots:
Tour.module example

Future work plan:
- Update plugin to take a route path so only page specific tours are in markup.
- Config entity to also provide yaml and possibility adding a ui to add new tour items.
- Derivative plugin class.
- Start converting all core modules that implement hook_help (via follow ups).

nick_schuch’s picture

FileSize
75.14 KB

Images (cannot use via external service).

cweagans’s picture

I don't think we should be looking at getting rid of hook_help here. The purpose of the tour module should be a user-facing onboarding that can easily be disabled (leaving the hook_help text in place). A UI for defining tour steps is, IMO, also out of scope here (that should be handled in contrib).

Your code looks pretty nice. One thing that immediately came to mind: is the tour text translatable? I have no idea how that works from within a plugin definition. Also, how do you handle ordering? Or multi-page tours (that is, go through a few UI elements on one page, go to the next page, go through a few UI elements, etc)? (I understand that this is a WIP - if those things are not already implemented, do you have a plan for doing so?)

tim.plunkett’s picture

+++ b/core/modules/tour/lib/Drupal/tour/Plugin/tour/tour/DrupalPowered.phpundefined
@@ -0,0 +1,32 @@
+ *   label = @Translation("Drupal Powered"),
...
+ *     "value" = "<h2>Powered by Drupal!</h2>",

The first line will be translated. The second will not, but could be if it was wrapped in @Translation

cweagans’s picture

Would it be better to split the header and body into two separate fields? Then the header can always be rendered with an h2 and the body can always be in p tags and we won't have to worry about including the markup in the translated text?

nick_schuch’s picture

Thanks for the input guys!

I also believe the UI can be handled later in contrib.

They are currently not yet implemented. However I plan to have them available.
1) As tim.plunkett has suggested we can go with 2 values (title and body) to handle translation.
2) In terms of ordering (what tour item comes first?) and grouping (spans over multiple pages) I was going to handle these via respectively named values on the plugin.

markwk’s picture

Cool to see this discussion for core. Here is my contrib route http://drupal.org/project/joyride

larowlan’s picture

Status: Needs work » Needs review
FileSize
75.1 KB
19.16 KB

So this patch adds config entities support for defining the tips via a derivative plugin.
Next stop is weights/sorting and then test coverage.
Basically this means modules can add config/tour.tooltip.foo.yml and have their stuff just work.
Sample yml file

name: menu-en
title: Administer your site
langcode: en
body: Use the menu to find and perform common administration tasks.
paths:
 - <front>
 - admin/*
attributes:
  data-id: toolbar-tab-administration
  data-text: Next
  class: custom

Status: Needs review » Needs work

The last submitted patch, tour-module-1809352.27.patch, failed testing.

jessebeach’s picture

nick_schuch and larowlan pinged me in #contribute about overlay issues. I can't determine from the issue what those are, but I'll help in any way I can.

larowlan’s picture

Next patch will need review of the JavaScript
Have reached out to a11y group for their input into the best markup.
Tagging accordingly

nod_’s picture

Missing dependencies in the library_info hook

$libraries['tour'] = array(
    'title' => 'Tour',
    'website' => '',
    'version' => '',
    'js' => array(
      drupal_get_path('module', 'tour') . '/js/tour.js' => array(),
    ),
    'dependencies' => array(
      array('system', 'jquery'),
      array('system', 'drupal'),
      array('tour', 'jquery.joyride'),
    )
  );

More readable JS with some new core things added (and the behavior name was wrong):

(function ($, Drupal) {

  "use strict";

  Drupal.behaviors.tour = {
    attach: function () {
      var $tour = $('#toolbar-tab-tour').once('tour');
      var $tooltips = $('#tour-tooltips');

      function startTour (e) {
        $tour.addClass('active');
        $tooltips.joyride({
          'postRideCallback': function () {
            $tour.trigger('click');
          }
        });
      }

      function stopTour (e) {
        $tooltips.joyride('destroy');
        $tour.removeClass('active');
      }

      if ($tour.length) {
        $tour.toggle(startTour, stopTour);
      }
    }
  };

})(jQuery, Drupal);
jessebeach’s picture

It looks like the patch in #27 was created from a merge between two branches with different histories. I'm getting numerous rejected hunks when applying and I can't imagine why this patch would alter the core CHANGELOG like this:

diff a/core/CHANGELOG.txt b/core/CHANGELOG.txt  (rejected hunks)
@@ -37,6 +37,7 @@ Drupal 8.0, xxxx-xx-xx (development version)
       * Profile
       * Trigger
 - Removed the Garland theme from core.
+- Removed the Statistics module's accesslog functionality and reports from core.
 - Removed backwards-compatibility with 'magic_quotes_gpc'/'magic_quotes_runtime'
   PHP configuration settings. Both are required to be disabled.
 - Universally Unique IDentifier (UUID):

larowlan, I'm happy to do an a11y review, but can you verify that the latest patch is correct? And if so, it needs a reroll.

nick_schuch’s picture

Hi jbeach!

Myself and larowlan are currently working out of a branch in a sandbox. Will have a patch for review very soon.

larowlan’s picture

Status: Needs work » Needs review
FileSize
67.68 KB

This patch adds some a11y fixes to joyride plugin (see https://github.com/zurb/joyride/pull/69)
Adds full test coverage.
Adds 'location' support for positioning the tour items.
Updates for the new hook_toolbar format.
Also adds a tour for the views ui to demonstrate the real power here.
Various fixes and cleanups.

Some issues with overlay interactions exist, first load in overlay works however after the overlay is closed it needs an extra click to fire.
Hoping to get help from jessebeach and nod_ on that front

Latest code is in git.drupal.org:sandbox/larowlan/1790736.git 'tour' branch (nod_ you still have commit access from the modals issue)

mgifford’s picture

I applied the patch. It's quite nice. There's just the one example now, right? I'm curious who to add to this.

Great that you've improved joyride. I hadn't played with it before, but always good to work with the external communities.

Trying this with a VoiceOver there were a few things that could be improved. I could enable Tour and I could hear what was being announced, but there was no context. Was also useful to be able to close it easily.

If the tab focus has shifted to the example of the search box, then one should be able to just go to the next item (via tab) and be placed in the context of the search box. Tabbing from the tour box jumped me up out of context from the tour which made it not very useful.

I can see this being really useful, but we need to figure out how to have it make sense for non-sighted users.

tsvenson’s picture

@larowlan asked me to UX test the patch in #34 and try the Views tour. Here is my quick test results.

Firstly, the tour did not work with overlay. Had to remove the overlay part in the URL to get it working

I also find the "Close" button is confusing. Particularly when I tried it on the start page which only has one tour item and thus it says Close directly. I expected the tour would have more steps than that.

A better choice would be "End Tour".

The Views tour did have more steps, but there where no indication of how many. Would it be possible to complement "(x of y)" in the lower right corner?

That would then also help to make more sense of "End Tour" when on the front page it would then have shown "(1 of 1)".

larowlan’s picture

thanks @tsvenson and @mikegifford

@mikegifford - there is an example for the views ui, enable the 'front page' view and click edit on it. Note that it doesn't work in overlay but is getting close. I agree re sand-boxing the tab to the tip dialog and it's target, we should be able to do that with js easily enough.

@tsvenson, I agree 'End tour' is better. Thanks. We should be able to add the x of y easily enough.

Awesome feedback - reviewers++

mgifford’s picture

I didn't see the Tour Menu item show up here -> admin/structure/views/view/frontpage/edit

Also in the menu, the contrast on the "?" makes it disappear when selected. That's not the same UI pattern as the other top menu items.

Quite excited about this though. Will be a great way to to a tour of basic functionality. Particularly if contrib modules start adding on their own tours. Wow. Maybe we won't have to be guessing so much about where to go to figure out how a new module is supposed to work.

larowlan’s picture

Latest screencast http://youtu.be/oP3xc4qSzdc

In the sandbox we have resolved the following
* Thomas's request to use 'End Tour'
* Thomas's request to use x of y
* Mike's request to sandbox tabbing to the tour item (also pushed up to joyride pull request)
* Overlay working with subsequent re-opening

Remaining issue
* Overlay working after navigation from one overlay page to another

tsvenson’s picture

@larowlan: Video looks great and the additions of the step numbers did improve the UX.

One other thing I noticed while testing the patch in #34 was that if you start the tour on the start page, then leave it and click on something that will open the overlay, the tour will be left open behind the overlay. As that patch didn't work with the overlay, I couldn't test what happened with the view tour in that case. What I did notice, though, is that the open tour behind the overlay does not close when clicking the tour button again. It does if I close the overlay first though.

Steps to recreate:

  1. Open start page
  2. Click Tour to open the one tour step for the page
  3. Click Structure to get the overlay
  4. Notice that the start page tour is still there
  5. Click Tour, notice it wont close
  6. Close the overlay
  7. Click Tour to close the open step

I don't see any real UX problems with this, just thought it was worth mentioning as I have no idea what happens when there are two tours open, one behind and one in front of the overlay.

jessebeach’s picture

The joyride plugin is setting position:relative on the body tag, which isn't strictly necessary to do the positioning, but there we go. The result is the toolbar tray now jumps to the left edge of the its position context, what had been the HTML doc, and is now the body.

Screenshot of the menu toolbar tray open. The tray contents appear to be shifted 100% of their container width to the right.

The toolbar should be able to handle a relatively positioned body tag. This patch sort of solves the issue. #1855208-10: Toolbar tray needs to fix to top of screen on scroll.

The issue will still be present on wider touch devices, but I think we can extend the solution in the patch above to not just touch device, but all wide devices. It'll make the tray placement more robust in all cases.

Getting the patch above committed is a good first start.

jessebeach’s picture

This feature will need some additional accessibility work.

It's true that the tooltips receive focus and one can tab through them when a tour is enabled. Turning on ChromeVox, however, results in no information about the tooltip feature being announced nor any information in the tooltips. We'll need better aural announcements for this to pass an a11y gate.

I can help with this. It's solvable.

We can add some help text in an aria-live region for the toolbar tab. This isn't possible in a structured way at the moment, but we've had plans to make the toolbar APIs more accessible for this kind of purpose.

#1860434: Refactor the Toolbar JavaScript to use Backbone; fix several poorly functioning behaviors

I'm not sure the best way to make the tooltip content more obvious. Maybe falcon03 can give us some suggestions.

jessebeach’s picture

Oh, and in my two critical comments above, I neglected to mention that I really like this feature. Awesome work. Sorry to point out areas that need work without mentioning so much is already quite fantastic.

effulgentsia’s picture

For anyone playing along at home, here's an up to date patch from the latest commit in the sandbox, and interdiff relative to #34.

larowlan’s picture

Status: Needs review » Needs work

Thanks @effulgentsia, to describe the latest patch - this provides

  • aria-describedby and aria-labelledby for the dialogs, this doesn't seem to work on voiceover but I'm aware that voiceover doesn't always play nice. Would love if someone with jaws/windows could test this. The markup looks right according to the specs and @Christian Biggins and I have been looking at this for some time with no joy from Voiceover.
  • Trying to get the overlay to work after subsequent page-loads. there is still a bound click event that needs to be unbound here for this to work

Can we make sure @'Christian Biggins' gets some commit credit here as he's spent at least an hour with me looking at this.

effulgentsia’s picture

Status: Needs work » Needs review

This looks awesome. I leave it to front-end developers to review the JS and CSS. I took a look at the PHP code, and in general, it's very nice and clean. I'm curious though why there's a need for both the TourTooltip config entity and everything related to the Tooltip plugin (including TourManager, TourInterface, etc.). What exactly are we wanting to be pluggable? From just an initial glance at this, it seems to me we could keep the config entities, drop the plugins, and support all needed customization via the theme functions and their preprocessors, but maybe there's some important use cases I'm not thinking of?

effulgentsia’s picture

#44: tour-module-1809352.44.patch queued for re-testing.

effulgentsia’s picture

I'm currently in the US, not with everyone at DrupalCon Sydney, so I'm going to sleep now, and will check back in on this in the morning. If this manages to get to RTBC during the Sydney code sprint, I have no objections to it being committed with the current config entity / plugin architecture: any desired refactoring of that could easily be follow up material.

Ralt’s picture

Hi,

I made a little review of the JS part.

+++ b/core/modules/tour/js/tour.jsundefined
@@ -0,0 +1,115 @@
+      function setupTour(items, scope) {
+        console.log('setting up');

console.log should be removed from production.

+        if ($(items).length) {
+          $(toolbar, scope).removeClass(toolbar_class);

$(toolbar, scope) is used several times. It should be stored somewhere.

+
+          // Loop over items and remove missing tour items.
+          $(items + " li").each(function( index ) {

$(items + " li") should be $("li", items). It's also used several times, so subject to caching.

+            var tour_item_id = $(this).attr('data-id');
+            var tour_item_class = $(this).attr('data-class');
+            if (!$("#" + tour_item_id).length && !$("." + tour_item_class).length) {
+              $(this).remove();
+            }
+            else {
+              $(this).attr('data-text', Drupal.t('Next'));

$(this).data('text') should be used instead of $(this).attr('data-text'). Same for $(this).attr('data-id') and $(this).attr('data-class').

+            }
+
+          });
+
+          // Update the last item to have "Close" as the button.
+          $(items + " li").last().attr('data-text', Drupal.t('End tour'));
+        }
+        else {
+          $(toolbar, scope).addClass(toolbar_class);
+        }
+        $(toolbar, scope).bind('click.tour', function() {
+          if ($(overlay).length || !window) {
+            return false;
+          }
+          if ($(toolbar + ".touring", scope).length) {
+            $(items).attr('hidden', true).joyride('destroy');

$(items).attr('hidden', true) should be $(items).prop('hidden', true). Also, the hidden property should be used with care, since it can be overriden by display: block;.

Besides, shouldn't simple quotes/double quotes usage be unified?

+            closeTour(scope);
+          }
+          else {
+            $(toolbar, scope).addClass('touring');
+            $(items).attr('hidden', false).joyride({
+              'postRideCallback': function() {
+                $(toolbar, scope).removeClass('touring');
+              }
+            });
+          }
+          return false;
+        });
+      }
+
+      /**
+       * Cleans up the tour.
+       */
+      function closeTour(scope) {
+        var toolbar = "#toolbar-tab-tour";
+        $(toolbar, scope).removeClass('touring');
+      }
+
+      /**
+       * Detaches up the tour.
+       */
+      function detachTour(scope) {
+        var toolbar = "#toolbar-tab-tour";
+        closeTour(scope);
+        $(toolbar, scope).unbind('click.tour');
+      }
+
+      /**
+       * Initialize the appropriate tour.
+       */
+      if ($(overlay_content).length) {
+        // Remove tour events associated with overlay.
+        detachTour(window.parent.document);
+        // Attach it based on outer content.
+        setupTour(overlay_content, window.parent.document);
+      }
+      else {
+        $(document).bind('drupalOverlayBeforeClose', function() {
+          // Remove tour events associated with overlay.
+          detachTour(window.document);
+          // Attach it based on outer content.
+          setupTour(tour_items, window.document);

detachTour() and setupTour() are used twice together. Shouldn't there be a restartTour() method?

+          console.log('closing');
+        });
+        $(document).bind('drupalOverlayBeforeLoad', function() {
+          // Remove tour events associated with overlay.
+          detachTour(window.document);
+          console.log('closing before move');
+        });
+
+        // Don't run this in overlay.
+        // This only runs once in the parent.
+        if (window == window.parent) {
+          $('body').once('tour', function() {
+            setupTour(tour_items, window.document);
+          });
+        }
+        else {
+          setupTour(tour_items, window.document);
+        }
+      }
+    }
+  };
+
+})(jQuery, Drupal);

Regards,
Florian

Ralt’s picture

Status: Needs review » Needs work
Wim Leers’s picture

Very, very nice work! :) It works great already!

I'm missing one important feature though: the ability to have tours that cover multiple URLs/pages, i.e. that are not only tours for the current page, but that guide you through important features of Drupal as a whole, thus taking you to different URLs.


In my quick review, I tried to not cover the same grounds as the reviews in #46 and #49. Unfortunately, I see many, many small issues. I am unable to do a deep review because there's still too many small rough edges, and because #46 is not yet answered. I think the review below will be useful though :)

+++ b/core/modules/tour/js/tour.jsundefined
@@ -0,0 +1,115 @@
+    attach: function(context) {

s/function(context)/function (context)/

See http://drupal.org/node/172169.

+++ b/core/modules/tour/js/tour.jsundefined
@@ -0,0 +1,115 @@
+      var overlay = "#overlay-container";

1. Don't start querying the DOM if you don't really have to.
2. Don't make assumptions about other modules' DOM structures if you don't have to.

In this case, there's Drupal.overlay.isOpen that you should use to check whether the overlay is open.

+++ b/core/modules/tour/js/tour.jsundefined
@@ -0,0 +1,115 @@
+          $(items + " li").each(function( index ) {

s/function( index )/function (index)/

See http://drupal.org/node/172169.

+++ b/core/modules/tour/js/tour.jsundefined
@@ -0,0 +1,115 @@
+          // Update the last item to have "Close" as the button.

s/Close/End tour/

+++ b/core/modules/tour/js/tour.jsundefined
@@ -0,0 +1,115 @@
+          console.log('closing');

console.log()s should be removed.

+++ b/core/modules/tour/lib/Drupal/tour/Tests/TourTest.phpundefined
@@ -0,0 +1,168 @@
+    // Naviate to tour-test-1 and verify the new tip is found.

s/Naviate/Navigate/

+++ b/core/modules/tour/lib/Drupal/tour/TourBase.phpundefined
@@ -0,0 +1,88 @@
+abstract class TourBase extends PluginBase implements TourInterface {
+  /**
+   * Weight of the tour item.

Newline between these.

+++ b/core/modules/tour/lib/Drupal/tour/TourBase.phpundefined
@@ -0,0 +1,88 @@
+   * Language of the tour.
+   * @var string

Newline between these.

+++ b/core/modules/tour/lib/Drupal/tour/TourBundle.phpundefined
@@ -0,0 +1,27 @@
+   * Overrides Symfony\Component\HttpKernel\Bundle\Bundle::build().

Missing leading backslash.

+++ b/core/modules/tour/tests/tour_test/config/tour.tooltip.tour-test-1-it.ymlundefined
@@ -0,0 +1,11 @@
+label: La pioggia cade in spagna ¶

Trailing space.

+++ b/core/modules/tour/tour.api.phpundefined
@@ -0,0 +1,12 @@
+ * Describes api functions for tour module.

s/api/API/

+++ b/core/modules/tour/tour.api.phpundefined
@@ -0,0 +1,12 @@
+ * Allows modules to alter tour items before render.

Should document the parameters.

+++ b/core/modules/tour/tour.api.phpundefined
@@ -0,0 +1,12 @@
+  ¶

Trailing space; should contain sample code.

+++ b/core/modules/tour/tour.moduleundefined
@@ -0,0 +1,197 @@
+  // Library definition for tour.

This comment can go away.

+++ b/core/modules/tour/tour.moduleundefined
@@ -0,0 +1,197 @@
+      drupal_get_path('module', 'tour') . '/js/tour.js' => array(),
+    ),
+    'css' => array(
+      drupal_get_path('module', 'tour') . '/css/tour.css' => array(

Instead of calling drupal_get_path() many, many times, call it once, store it in a variable, reuse that.

+++ b/core/modules/tour/tour.moduleundefined
@@ -0,0 +1,197 @@
+        'type' => 'file',

type = file is the default, no need to specify that here.

+++ b/core/modules/tour/tour.moduleundefined
@@ -0,0 +1,197 @@
+  // Library definition of joyride.

This comment can go away.

+++ b/core/modules/tour/tour.moduleundefined
@@ -0,0 +1,197 @@
+      // Require jQuery core by System module.
+      array('system', 'jquery'),
+      // Require jQuery cookie by System module.
+      array('system', 'jquery.cookie'),
+      // Require custom tour library.

These comments can also go away.

+++ b/core/modules/tour/tour.moduleundefined
@@ -0,0 +1,197 @@
+function theme_tour_items($vars) {

Why can't you use theme_item_list()?

+++ b/core/modules/tour/tour.moduleundefined
@@ -0,0 +1,197 @@
+function tour_page_build(&$page) {
+  $page['#attached']['library'][] = array('tour', 'tour');

You're already attaching the library in hook_toolbar(), why is it then also unconditionally loaded via hook_page_build()?

larowlan’s picture

Hi wim and effulgentsia,
wim's request for a multi page tour is the answer to effulgentsia's question. We have the interface and the plugin so that contrib can provide a multi page plugin, or anything else for that matter.
Thanks for these detailed reviews
Reviewers ++
This will get a reroll during the sprint tomorrow.
Lee

tim.plunkett’s picture

This architecture is the complete opposite of every other ConfigEntity/Plugin pair. Elsewhere, a ConfigEntity contains one or more plugins, ideally with PluginBag. This has a Plugin storing the entity.

In addition, TourManager::getByPath() has its own caching, separate from the CacheDecorator, with no cache bin of its own, and several drupal_container() calls.

+++ b/core/modules/tour/lib/Drupal/tour/TourManager.phpundefined
@@ -0,0 +1,79 @@
+    foreach ($this->getDefinitions() as $key => $definition) {
+      $item = $this->createInstance($key, $definition);

+++ b/core/modules/tour/tour.moduleundefined
@@ -0,0 +1,197 @@
+  $items = $manager->getByPath(current_path());

This creates an instance of every tour plugin, always.

+++ b/core/modules/tour/tests/tour_test/lib/Drupal/tour_test/Plugin/tour/tour/TestTour.phpundefined
@@ -0,0 +1,54 @@
+ *   module = "tour_test"

You use the module key, but you don't use the ProcessDecorator to actually filter them out. This means there is no test coverage for gathering tour plugins while tour_test is disabled. They will, but shouldn't, be found by simpletest.

+++ b/core/modules/tour/tests/tour_test/tour_test.infoundefined
@@ -0,0 +1,5 @@
+name = Tests for tour module
+hidden = TRUE
+core = 8.x
+description = Tests for tour module
+dependencies[] = tour

Missing package

+++ b/core/modules/tour/tour.api.phpundefined
@@ -0,0 +1,12 @@
+ * Allows modules to alter tour items before render.

Hook docblocks are declarative, so "Allow modules..". It should go on to explain why/how to do this, and also document the parameters

+++ b/core/modules/tour/tour.api.phpundefined
@@ -0,0 +1,12 @@
+function hook_tour_items_alter($tour_items, $path) {
+  ¶

I have no idea why you would want or need to alter these, and this should give an example.

+++ b/core/modules/tour/tour.moduleundefined
@@ -0,0 +1,197 @@
+ * Implements hook_tour_info().
+ */
+function tour_tour_info() {
+  return array(
+    'tour' => array(
+      'title' => 'Tooltip',
+      'description' => 'Generic tooltip class',
+      'class' => 'Drupal\tour\Plugin\tour\tour\Tooltip',
+      'derivative' => 'Drupal\tour\Plugin\Derivative\Tooltip',
+    ),
+  );

Where is this invoked?

+++ b/core/modules/tour/tour.moduleundefined
@@ -0,0 +1,197 @@
+  uasort($items, array('\Drupal\tour\Plugin\Core\Entity\TourTooltip', 'sort'));

This is wrong, you cannot use the hardcoded class name like that. Also, you're calling the entity sort on the results of the tour plugin manager? That doesn't make sense.

+++ b/core/modules/tour/tour.moduleundefined
@@ -0,0 +1,197 @@
+  $variables['page']['help']['tour']['#markup'] = theme('tour_items', array('tour_items' => $items));

use '#theme' => 'tour_items', not '#markup'

effulgentsia’s picture

This architecture is the complete opposite of every other ConfigEntity/Plugin pair.

That doesn't necessarily make it wrong though. To determine whether it makes sense or not, it would be helpful to see what a multi-page tour plugin would look like. Please add one during you next code sprint if you're able to. I'm assuming it would be valuable to have one in the module itself, but if not, then at least in the test module. @tim.plunkett is right that elsewhere in core, we always have a single ConfigEntity type with the plugin(s) "inside" it. For example, all Views use the same config entity type, but each one can consist of different Display plugins. That allows all Views to be listed together in an admin listing page. If the same is desired here, then we should follow the same pattern. However, if we want the single-page plugin and the multi-page plugin using different config entity types for their configurations, and don't want single-page entities and multi-page entities listed in the same admin listing, then the pattern that's here of the plugin as the "outside" object and the config entity as the "inside" object might be appropriate, unless someone knows of why it wouldn't be.

CB’s picture

I've spent a bit of time going over this from an accessibility viewpoint this morning, I'm not done yet but some notes;

  • The rendered code uses an ordered list to contain the pages tooltips, but I think Joyride kills the list when it builds the tooltips, so I don't think that will be a concern.
  • I have no access to a windows machine where I am, my VM is playing up so not able to test with Jaws/NVDA but Voice Over reads the tips as they appear and follows the tips around the page for multi-tipped pages. Notes;
    • Each new tooltip is announced correctly using the tooltip label (aria-labelledby)
    • Focus on new tooltips goes immediately to the control in the next tooltip; from a UI perspective this is correct functionality, but it requires a VoiceOver user to select the tooltip content and title in reverse order. I think thats fine.
  • Keyboard navigation is constrained within the tooltip - ie, tab key will not exit the tooltip. A nice touch for keyboard nav.
  • The only thing I think I'd like to see, if possible, is the 'Tour' link to receive focus again when the tour is complete - Keyboard navigation is severed on completion, focus is lost.

Super nice module though. Double thumbs up.

Grayside’s picture

I have a use case for Tours to have content populated via external API call. We use a central site to allow things like tour and help tooltips to be managed as first-class content for our customer/docs team, and not require code deployment steps to go live.

effulgentsia’s picture

In that use case, do you have any configuration associated with it (such as the URL of the web service to call), or is it completely driven by code, and configuration free?

effulgentsia’s picture

Also, the configuration files in the latest patch here include things like 'paths', 'langcode', and 'attributes'. Would your use case still have that living in config files, or would you be controlling these entirely from code as well?

tsvenson’s picture

I have applied the patch in #44 and done some more UX testing.

One major thing we need to discuss is if "?" needs to be changed in the toolbar:
toolbar_look_questionmark.png
For most users the "?" mean help. However, the Tour module is providing a demo, or guided tour, for the current context.

Also, it should only be visible when there actually exists a tour for the current context. At least it should be ghosted and not possible to click on when there is no tour.

I'm also contemplating about if mixing it with the other toolbar options as it is now is optimal.

Would it be possible to instead have it right aligned in the toolbar?

I also note that Tour is now working with the overlay and it seems that the the scenario I gave in #40 is being worked on.

However, there seem to be a few issues left there to iron out.

Killing the Overlay:

  1. Click on Structure in the overlay to open it
  2. In the structure list click Tour
  3. That will result in that the overlay is killed

If you repeat the above, but first click Tour on the start page (so the sole tour item is visible), then both the overlay will be killed and the tour closed.

If you then take it one step further and open a View in edit mode and click Tour, a second tour will be opened. So at least not a conflict here.

Seems there is some more work here needed.

I think the easiest solution here is that any click that result in an action outside the Tour object will close it. That is the behaviour I have seen on most sites using this kind of demo has. Thus most users will already be used to that happening.

Autoscroll

I think there needs to be some adjustments to the autoscroll of the page depending of the tour step opening above or below what it points to.

Step 4 in the Views edit is an example of this. It scrolls down the page but leaves about the same space above and below. In this case it works, but in other cases the space wont be enough.
tour_step_below.png
My proposal here is that if the step is opened below (pointing up) is that it should scroll to show most space above so that I can see more of the context it is pointing too.

The same when it is pointing down, then it should show most space below to reveal more of the context there.

Here is also a good illustration where the Tour step should have been pointing down instead:
tour_step_pointing_wrong.png
While it does point to the button (OK, a little misplaced) a better choice would have been to place it above pointing down as more of the context would have been shown.

The text in this case could then also have included the "Auto preview" option and thus the step could have been place just above that.

Besides these things, you guys have done some awesome work here. Its really impressing and the latest UX improvements have helped to take it to an even higher level.

tsvenson’s picture

Can this be used on live sites too?

When I do a git status I see lines like:

core/modules/tour/config/tour.tooltip.admin-content-en.yml
core/modules/tour/config/tour.tooltip.search-en.yml
core/modules/views/views_ui/config/tour.tooltip.views-ui-active-display-en.yml

Which, after reading the content, I assume is the actual Tour steps.

I'm not so sure if the search tip is right to have in the tour module. I see that as a tour-tip for the production site.

This is actually a very useful functionality to use on production sites too. Many sites are starting to use this to highlight new features that has been added since the last time you where on that site.

Thus, I think it would be great if that too could be supported by this module. But I believe that requires maybe some changes to the format, such as a date keyword and an option if the tip should be automatically displayed when a user hasn't seen it yet (comparing last login with the date keyword for example).

The question then is where to store these yml-files. Good candidates for this might be:

sites/example.com/tour
sites/default/tour
sites/all/tour

If we can come up with a smart folder structure about this, then it should be possible to create an UI for the Tour module to be able to create these tour-tips in too. That is probably best to leave for contrib though so it has more freedom to mature for D8.

I also believe it would be worth looking into that the tour module, with very small adjustments, also can be used for context help similar to what the Advanced Help module does for inline "(?)" help in Views for example.

This not just for the backend, but also possible to use as inline help on the frontend too.

nick_schuch’s picture

First an foremost, thankyou everyone for a detailed reviews over the past few days!

After the Drupalcon Sydney sprint today we have been able to knock out the majority of the items in #49, #51 and #53.

Still left from these:

- Test for when 'test_module' module isn't installed as per #53.

So after looking through the recent reviews we also need to do the following:

- Tour module needs a new icon.
- Tour icon to either be ghosted or not visible (this is implemented in a basic form, but we should give this some more attention).
- Float the tour item to the right (just like edit)
- Clicking other items on the page closes the tour.
- Autoscroll, I believe this is due to the tray in the toolbar. I have also seen this on other items. We should close the toolbar tray.
- Triggers for when tour starts, changes and stops (suggested by larowlan).
- Additional store for yml files for uses (production sites).
- Tracking of what a user has seen (new tooltips added since).
- Tracking of which tooltips have been viewed by the user.
- Experience and role system.

Any chance that we could get these followed up in tasks? I have no problems creating all the follow up tasks.

This is awesome!

sun’s picture

Status: Needs work » Needs review

I only briefly followed this issue via e-mail notifications and may have missed earlier discussion details. I've also read the current issue summary. I did not look at the latest patches, but I assume they are still similar to earlier patches. (Sorry if I'm missing substantial details — in case I do, that means we need a new/better issue summary.)

Some critical points:

1) Why is this configuration? All that I'm seeing are declarations/definitions. That's not configuration. It's only configuration if we expect tour.module to have an administration interface and to allow to delete all existing tour step items, so as to allow you to create a custom tour, completely independent of the modules/extensions that you have installed. Bear in mind: Configuration is staged across sites. I cannot see why this declarative data should or would have to be staged. Configuration also needs to be (manually) translated. This usage does not really map to configuration IMHO. Instead, we have a (unlimited) set of tour step definitions, which are available, and for each user, we need to track which step has been read/seen already. In other words, it's an opt-out facility with a never-ending amount of items to opt out.

2) Why is there a "Tour" tab in the toolbar? (screenshots contained this) If tour.module is enabled, and if the current user did not opt-out, then the tour should be an inherent part of the user interface, and not something that needs to be specifically triggered or enabled. As a stellar example, I want to refer to the latest stable release of Wordpress, which ships with a tour functionality built-in. Wordpress even goes one step further and informs existing users that updated to the current version about changes in the UI/application, so they can make sense of new interface functionality as well as finding equivalents of removed things. Again, pretty exceptional. Anyway, back to the point: Why is there a "Tour" tab in the toolbar?

3) Does the architecture allow for different user role categories? Or alternatively, different user experience levels? The common point where "Did you know?" facilities break very badly is when you have a system of varying user experience levels. Which is very typically the case in Drupal. Novice users need to be trained on very basic operations. But for advanced users, >90% of those novice hints are annoying and disturbing; up to a point at which users get frustrated and desperately want to disable them entirely. We do not have to provide a concrete implementation and solution right now, but IMHO, the architecture should be prepared for "experience levels". The most simple/silly way to do that would be to define a range of 1 to 100, and every tour step definition has a "level" property with a value in that range. Since this initial implementation targets novices only, all definitions would have a $level of 1.

Thanks for working on this. Keep on rocking!

CB’s picture

FileSize
14.98 KB
5.94 KB
1.55 KB

Some ideas for icons;

Bus tour icon

Signpost tour icon

camera tour icon

Camera is probably my pick.

nick_schuch’s picture

Thanks for the critical points sun! The latest patch has really been cleanup so no dramas (worked on since #20).

1) We will be looking at providing a configuration interface through contrib. We might like to have tooltips staged across sites (possibly create a tooltip that references a point in content). I also definitely feel that it is worth tracking with tooltips have been viewed and some description of an opt out per module.

2) We are using the "Tour" item in toolbar as a common toggle item (taking the same approach as the "edit" toggle). It only gets displayed when tooltip items are on the page. Im watching the "Edit" toolbar items very closely to make sure this is consistent.

3) I think based on roles/experience is a solid idea! We want new users to be eased into Drupal and experienced users to be reminded. We should definitely get the discussion going for this.

Ive added these items into #61 so follow up issues can be created (and these are all tracked).

jwilson3’s picture

As for icon ideas, I would be more inclined to a "signpost" icon (Eg, google search for signpost icon image) or the traditional and universally understood tourist info icon: an "i" with a circle around it.

larowlan’s picture

Hi all, I think this one

Additional store for yml files for uses (production sites).

Can be removed from the to-do list.
A tour_ui module could be added by contrib for adding/editing/listing the configs and hence there would be no need to search an additional folder.

One more we should add is a status flag.
A tour_ui module would need to be able to disable/enable tour items as required.

Great idea sun re roles/experience levels.

larowlan’s picture

Status: Needs review » Needs work

Thanks Nick for your work on this.
Saw some whitespace issues, listing them while I think of it.

+++ b/core/modules/tour/js/tour.jsundefined
@@ -3,54 +3,53 @@
+				var toolbar = $(toolbar_id, scope);

whitespace

+++ b/core/modules/tour/lib/Drupal/tour/Plugin/Core/Entity/TourTooltip.phpundefined
@@ -67,6 +69,13 @@ class TourTooltip extends ConfigEntityBase {
+  ¶

whitespace

+++ b/core/modules/tour/tour.api.phpundefined
@@ -1,12 +1,23 @@
+ ¶

whitespace

+++ b/core/modules/tour/tour.moduleundefined
@@ -169,15 +122,43 @@ function tour_page_build(&$page) {
+  ¶

whitespace

+++ b/core/modules/tour/tour.moduleundefined
@@ -169,15 +122,43 @@ function tour_page_build(&$page) {
+  ¶

whitespace

jwilson3’s picture

+++ b/core/modules/tour/js/tour.jsundefined
@@ -3,54 +3,53 @@
+				var toolbar = $(toolbar_id, scope);
 

Also, visible from the review in #67: prefix variables that point to jQuery objects with a dollar sign.

nick_schuch’s picture

Status: Needs work » Needs review
FileSize
71.46 KB
3.22 KB

This resolves the issues in #67 and #68.

mgifford’s picture

Tested this latest patch here:
http://simplytest.me/project/drupal/8.x

Really like how it works with Views. Very useful!

What else is needed to get this into core?

nick_schuch’s picture

Thanks mgifford!

I believe im up to the last item as listed in #61 which relates to this:

You use the module key, but you don't use the ProcessDecorator to actually filter them out. This means there is no test coverage for gathering tour plugins while tour_test is disabled. They will, but shouldn't, be found by simpletest.

Im currently in IRC with tim.plunkett trying to get it resolved.

tim.plunkett’s picture

FileSize
25.8 KB
72.65 KB

Because of the DerivativeDiscoveryDecorator, the ProcessDecorator hack isn't needed.

However, while debugging and writing a test for that, I decided to just go through and clean up everything I could see.

CB’s picture

NVDA reads the dialogs correctly still using IE (but not Chrome due to how it handles Aria - see bug)

Grayside’s picture

@effulgentsia/#57 We have code built around the tooltip library that introduces topics in markup. Requests to the help server (powered by a later, pending release version of the http://drupal.org/project/goodhelp module) provide it HTML. Configuration provides a base url for those requests and might be extended for credentials if we felt a need to lock down read access.

nick_schuch’s picture

FileSize
73.46 KB
4.08 KB

What this resolves:

- Addition of triggers (drupalTourBeforeOpen, drupalTourBeforeClose) for other modules to interact with tour modules js.
- Floated the tour toolbar item to the right to ensure it is consistent with edit.module.
- Toolbar item hides when overlay doesnt have tour related content to display.
- Fixed piling of click events as overlays are switched (one overlay to another).
- Ensured we are using the ariaId in our theme function as it is wrapped with drupal_html_id().

Where do we go from here:

The following discussions I feel we should have in separate followups:

- Tour module needs a new icon.
- Clicking other items on the page closes the tour.
- Autoscroll, I believe this is due to the tray in the toolbar. I have also seen this on other items. We should close the toolbar tray.
- Tracking of which tooltips have been viewed by the user.
- Experience and role system.

Looking back on this ticket and where we have come to I feel that we are in a really good place with this.
This doesn't include a multiple-page tour but does contain the necessary architecture (client and backend) for one to be written in contrib or added during feature freeze if the consensus was strong. For multi-page its a case of go from "page A" to "page B" this could be facilitated via:

- drupalTourBeforeOpen could be used to add a "Next page" option to the end of the tour and append a option to the page eg. "node/1?tour=1"
- Some javascript could be used on the receiving page to start the tour right away and continue.

This is a basic example that could need fleshing out but the backend is there to support this type of functionality.

It would be great to get some reviews and see what we might be missing and what we still need to get this over the line before feature freeze.

larowlan’s picture

Status: Needs review » Needs work
Issue tags: -Needs accessibility review +Needs manual testing
FileSize
29.4 KB
+++ b/core/modules/tour/js/tour.jsundefined
@@ -42,10 +42,16 @@
               "postRideCallback": function() {

I think we need a $(document).trigger('drupalOverlayFinished') in here too (inside the postRideCallback function) for other modules to know that the tour has completed.

Removing accessibility tag based on Mike and Christian's feedback.

Manually tested fine except for positioning when shown in overlay eg:

Screen Shot 2013-02-12 at 5.55.40 PM.png

I think we should address that now, it will be a position css issue with overlay.

The remaining issues I'm happy to be follow-ups, none of them will be major so we won't mess with thresholds.

I think you need to a line in Maintainers.txt to add a maintainer for tour.module.

+++ b/core/modules/tour/config/tour.tooltip.admin-content-en.ymlundefined
@@ -0,0 +1,13 @@
+body: Content can be filtered and bulk operated from this interface.

I know this is example content only, but that sentence needs work, I'm not sure 'bulk operated' is valid phrase. Suggest 'Use this interface to filter and perform bulk operations on your content' ?

+++ b/core/modules/tour/css/tour.cssundefined
--- /dev/null
+++ b/core/modules/tour/js/jquery.joyride-2.0.3.jsundefined

This is an MIT license, we need to make sure that it is ok for this to be included in a GPL project.

larowlan’s picture

larowlan’s picture

removing the position:relative from the body element from the joyride-2.03.css resolves the positioning issues.

larowlan’s picture

Status: Needs work » Needs review
FileSize
0 bytes
1.71 KB

Fixes issues identified in review at #76.
I think we just need the followups identified at #75 from here.
I don't think I should RTBC it as I've worked on it too much.

larowlan’s picture

FileSize
73.84 KB

Meh, wrong patch file

tsvenson’s picture

To follow up on @sun's 1st point in #62 I too think it is important to classify correctly what this is. Personally I don't see it as either content, nor configuration. In a way it is content, but it is at the same time not the same kind of content as text or files on a site. I believe the best is to classify it as documentation/help (I'll just call it documentation from here on).

Adding to that, I see two types of documentation:

  1. Platform documentation - About Drupal core and contrib/custom projects)
  2. Site documentation - About the site built using the platform

Sometimes this can be a little fuzzy, such as the existing search Tour tip. However, using the simple rule that if the tip is about a feature visitors to the live site uses, then it is a type 2 - Site documentation.

@sun also points out the issues with translation here when it is treated as config. This could lead to a big UX problem unless we come up with a better method. I took a peek in the tooltip.[blabla].yml in config_[blabla]/active and note that all the tour-tip files has been copied in there at install. I assume this is also the reason why I have to empty that folder each time I apply a new tour-patch, or?

As I outline in #60 this functionality could, with small adjustments, also be tweaked to be used as inline help in the same fashion as advanced help does for Views as well as inline help for the live site. In my view I see it as very beneficial for us to do so as it means we have one module that takes care of all of that. It would for sure be easier to maintain as well as work with and extend in contrib.

But we do need to solve where and how each tip/help is stored as well as make sure it is easy to translate too. When it comes to translation there are two main types:

  1. Code related - Core, contrib custom modules
  2. Site specific - Unique to the specific site

For the code related when it comes to projects hosted on d.o, it should use the same translation system as for any other translations for core and contrib projects. No need to build up something new there.

For site specific it needs to use the same translation tools as is used for site content and only stored local with support for staging to live site when needed.

larowlan’s picture

@tsvenson, a tour_ui module will cover all of these issues.
I think this belongs in contrib and will provide the ability to edit, enable and disable items.
This would involve creating a form/list controller for the tooltip entities and attaching them with hook_entity_info_alter().
I think it is too late to try to add a ui to core.

tsvenson’s picture

@larowlan, I'm fine with the UI living in contrib for D8. As I mention in #60 I actually prefer that for the UI so it has a chance to mature more easily than what the more strict rules for changes to core governs.

But it is also for the same reason we need to make sure this is done right for what is committed core. Once there and D8 is out, it will be difficult to make needed changes/improvements that was missed due to not making a proper UX analysis.

larowlan’s picture

I agree so I think we should be working on an iteration of tour_ui during the feature freeze -> code freeze period to identify and resolve any shortcomings

tsvenson’s picture

Assigned: Unassigned » nick_schuch
Priority: Major » Normal
Status: Reviewed & tested by the community » Needs review
Issue tags: -Needs accessibility review +Needs manual testing

Proposal to rename this module

For the last few days I've been debating with myself about the module name. Particularly in regards to how logical it will work if it also will be used for inline help for both the backend and frontend in Advanced Help style.

While the name "Tour" works well for the original intended use, It makes less sense when it comes to the inline help.

What I came up with in the end was "Guide". The tour is after all a Guided Tour and the inline help is Guided Help available in the context.

Then we can also use this to separate the two in folders as in:

  • guide_tour
  • guide_help

Those folders can live as subfolders to for example:

  • Modules
  • Themes
  • the /sites/* space

Site specific tour/help will then, of course, always live in the /sites/* space.

It should also, particularly needed for the "guide_help" items, be possible to override them for individual sites and contexts. This will especially be handy for default inline frontend guide_help such as the Search form.

Add "For more info:" links

I also believe it would be great to optionally be able to include links to more information about these items. That way it will be possible to make the text content shorter, while easily point the user to more detailed info.

Default here should be that all links open in a new tab/window. This as the user rarely want to lose the context they are in and in many cases they are also working on content. The "filter tips" link for text formats is a good example of how not to do this right.

cweagans’s picture

-1

Please don't complicate this issue. It should be very simple to take Joyride, drop it into core, and expose some nice tour steps as has already been done.

While Tour works well for the original intended use, It makes less sense when it comes to the inline help.

Don't scope creep this. Tour.module should not try to replace help.module. It should not be trying to provide inline help. It should be pointing out specific interface elements that help a user jump in and start working on the site.

Gábor Hojtsy’s picture

@tsvenson, @sun, et. al.: as long as the tour items are in configuration, they will be translatable as part of the software package on localize.drupal.org (just like exported views, default content types and fields, etc). I think treating tour items as (default) configuration is much more logical vs. content and we don't really have a third way, especially if we want to make it adjustable in contrib with more tours.

tsvenson’s picture

@Gábor Hojtsy: Thanks for clarifying that. Great to hear the multilingual initiative made that possible too.

Personally, though, I think documentation/help needs to be treated separately. Now some will live as config, while some will live as content and in the long run. Particularly for larger complex sites this will lead to similar issues as with the mix of config/content in the db created. But that's a different discussion to have.

Gábor Hojtsy’s picture

@tsvenson: I don't think this is within the scope here for several reasons.

There is already the help region for example in core, that lets you add page or section specific help/documentation with block visibility. That is a great tool to add custom help/instructions to sites too, but not as interactive as tour module will be :) And those blocks are stored as configuration in Drupal 8 too (and once custom blocks become content entities, they will be content entities exposed with configuration :). So I don't think in Drupal we can easily cut lines between docs and content when customisability is taken into account.

As for the translation question, guide/tour text in config will be exposed the same way on localize.drupal.org like help text written in hook_help() as well as any other code based thing. hook_help() is not separated from code either.

Wim Leers’s picture

Assigned: nick_schuch » Wim Leers

I'm working on an in-depth review + reroll with a lot of changes. I'm going to wrap it up in a few hours. Assigning to myself to prevent duplicate efforts.

tim.plunkett’s picture

@Wim Leers, I know that @nick_schuch and @larowlan and I had a big discussion about rearchitecting, you might want to pop on IRC to make sure you're not already duplicating some efforts.

Wim Leers’s picture

I just talked to tim.plunkett on IRC, and turns out they were discussing rearchitecting the PHP, I'm doing the JS :) Sounds like a perfect match, then!

(I've only changed the .module file a bit, haven't touched any of lib/Drupal/tour/*, which is likely precisely and only where they are working.)

tim.plunkett’s picture

@Wim Leers says he's working on the JS, we were talking about the PHP side of things.

Here is the prototype of the architecture I discussed with @larowlan and @nick_schuch:

interface TipPluginInterface {
  protected $dataID;
  protected $weight;
}

/**
 * Displays some text as a tip.
 *
 * @Plugin(
 *   plugin_id = "text",
 *   module = "tour"
 * )
 */
class TextTipPlugin implements TipPluginInterface {
  protected $label;
  protected $body;
}

/**
 * Displays a Youtube video as a tip.
 *
 * A possibility for contrib, not in core.
 *
 * @Plugin(
 *   plugin_id = "contrib_youtube",
 *   module = "contrib_youtube_tour"
 * )
 */
class YoutubeTipPlugin implements TipPluginInterface {
  protected $url;
}

/**
 * Contains a series of tips.
 */
class Tour extends ConfigEntityBase {
  protected $path;
  
  /**
   * The plugin bag.
   */
  protected $tipBag;
  /**
   * The array of plugin config, only used for export and to populate the $tipBag.
   */
  protected $tips;
  
  public getTip($id) {
    return $this->tipBag->get($id);
  }

  public getTipsList() {
    return array_keys($this->tips);
  }
}

$tour = entity_load('tour', 'views-admin');
foreach ($tour->getTipsList() as $id) {
  $tip = $tour->getTip($id);
}

Wim Leers’s picture

FileSize
17.85 KB
80.53 KB

Changes in rerolled patch

  • Less techy description in the .info file.
  • RTL CSS.
  • CSS clean-up: .joyride-tip-guide { z-index: 502; } was pointless, because it was being overridden by joyride-2.0.3.css… And I removed <code>.tour-no-items-hidden { display: none; } in favor of core's .element-hidden { display: none; }. However, due to toolbar.module's CSS, we currently still need an additional bit of CSS for that, see #1916690: Toolbar's CSS has too specific selectors for that.
  • The toolbar tab should contain a button, not a link, because you don't follow a link, you toggle a mode. Better for a11y, too.
  • The HTML5 "hidden" attribute is being used. This is not a core practice. It may well be appropriate, but I've changed this to use the more standardized .element-hidden class instead for now. I'm also not sure whether the "hidden" attribute is supported by all browsers. Furthermore, this doesn't need to be unhidden at all by the JS, because joyride does not show this HTML directly, it just parses it and renders it differently. To make it even worse, on top of all the preceding things, there was also #tour-items { display: none; } in the CSS, so the JS really absolutely pointless.
  • The tab was not being toggled to its "active" state. Many edge cases were not being handled yet (e.g. with a tour still open, open up an overlay, trigger a tour there, close the overlay, and variants of this). (Test this for yourself if you don't believe me.)
  • The JS was full of WTFs, because there are a lot of edge cases to think about (because we need to work with the Overlay). So I rewrote it, using Backbone, which makes it a *lot* easier to keep track of what's happening.
  • The two included tour tooltips set the class of the tooltip to "custom" for absolutely no reason, because there is no CSS to style it customly. Removed.

TODOs/observations

  • a11y of this is currently poor (both with and without overlay): when I start a tour, I expect you're unable to tab to anything except for the UI elements of the guided tour. This is not the case: I'm able to tab to other things in the page.
    That being said, I don't think this should be a commit blocker, since this is clearly non-essential functionality.
    Furthermore, this should be able to leverage the work for tabbing management we've been doing at #1913086: Generalize the overlay tabbing management into a utility library. There should be a follow-up issue for that.
  • hook_tour_tooltip_insert() and hook_tour_tooltip_update() are not documented in tour.api.php.
  • $variables['page']['help']['tour'], which renders to <div class="item-list"><ol id="tour-items">…</ol></div> ends up in a very strange place in the HTML: after <nav class="tabs" role="navigation"></nav> and before <div class="region region-content" />. In other words: in the middle of the page!
  • This sort of content is the perfect match for being lazy loaded. There should be a follow-up issue for loading all of the tour data asynchronously, only after clicking on the "Tour" tab/button in the toolbar.
  • If there's a tip for an element that does not yet exist on the page, and the user starts the tour, then the tips for non-existing elements will be removed. If the elements for the deleted tips are then AJAXed in, and the user starts the tour again, this causes the tips to not be available anymore (they've been removed).
  • As per the previous bullet, it's kind of senseless to generate the "1 of 2" etc. progress indicators on the server side. Similarly, why generate the "Next" button text or the corresponding "End tour" button for the last tour item on the server side? That's all HTML that's being sent over the wire for no reason. Related to this, why do we even want TourTooltip ConfigEntities to set data-text? Why can't we just always generate "Next" and "End tour" for the last item?
  • The ConfigEntity is called "TourTip" but in the code it's always "tour item". Please choose one and standardize on it, everywhere.
larowlan’s picture

Thanks @WimLeers
With regards to the hidden attribute, we had that so the screen readers didn't announce it. Might be wrong on that front.
With regards to tabbing, we had it so they could only tab through the elements in the tooltip, ie the buttons and the close. I assume this is no longer working.
Adding back the accessibility tags.
Much of the php stuff is being refactored so the class names will get a cleanup.

Wim Leers’s picture

#95:
- RE: hidden -> .element-hidden is actually used all over core for a11y purposes
- RE: tabbing: this already *was* broken in #80, I did not make any changes that could have deteriorated it; I've only added a slight enhancement in that the toggle is now a button instead of a link. Please spin up a demo of the patch in #80 (via this link: http://simplytest.me/project/drupal/8.x?patch[]=http://drupal.org/files/...) to convince yourself that this is in fact true.
- RE: a11y tags being added back: great :)

larowlan’s picture

As mentioned in irc, at #95 I'm not saying #94 broke tabbing, I'm saying it was working at one stage but clearly no longer is :)

Wim Leers’s picture

What's the status on the PHP side (as per #91 & #93)? What can I do to help this move forward? :)

larowlan’s picture

Hi Wim
@nick_schuch has refactored the php as per @timplunkett's suggestion - ie that the config entity represents a tour and wraps the tips (plugins) using a plugin bag.

The latest code is in the sandbox - we still need to restore:
*path matching, have been discussing adding that to entity_query with @chx (see http://privatepaste.com/c2576e2dcb and http://privatepaste.com/a928cf61dd) but most likely we will just load all entities and add a @todo to use entity_query() once config entity ng is in as $entity->path is an array and entity_query() does not support array properties (until config entity ng lands).
*aria labelled by/described by
*weight sorting is partially done
*export properties
*position support (we could position the tip left, right, above, below in the old patches)
*refactor tests to match new architecture

We haven't had a chance to incorporate your interdiff from #94, we've been focussing on the pluginbag functionality.

kudos to @nick_schuch cause I just couldn't get my head around pluginbags, but he did, and to @timplunkett for being patient with us.

@nick_schuch and I will be looking at this on and off over the weekend (it is Fri night here is Australia).

The sandbox is http://drupal.org/sandbox/larowlan/1790736 and the branch is tour, I've given you commit access.

Lee

nick_schuch’s picture

I have worked on the following (currently in the sandbox as per #99):
- aria labelled by/described by
- export properties
- positioning support
- refactor tests to match new architecture (halfway there).
- Implement .element-hidden on the tour items (instead of hidden via css).
- Cleaned up the yml files.
- Cleaned up refs of "tooltip" as we now us the term "tips".

We still have:
- weight sorting is partially done
- path matching (as per #99)
- finish off tests refactor.
- toolbar button to become a button.

Let me know if there is anything Im missing.

Nick

Wim Leers’s picture

Assigned: Wim Leers » Unassigned

#100: Some of the things you reference are already done in #94. Does that mean you're redoing them because the underlying code has changed to much, or why can't you just reroll my patch of #94 to apply to the sandbox?

(Also unassigning — forgot about that, sorry! I can't reassign it back to you, Nick.)

nick_schuch’s picture

Right you are Wim Leers.

We are now down to refactoring of tests in our branch. Myself and larowlan are both looking at these.

nick_schuch’s picture

Assigned: Unassigned » nick_schuch

I have refactored the tests. Will be having a chat with larowlan tomorrow to ensure we have covered all cases and what else needs a clean up before submitting a patch.

The current code can be found in the http://drupal.org/sandbox/larowlan/1790736 sandbox and the branch is "tour".

nick_schuch’s picture

Ok, here is the latest patch with the new refacted pluginbag architecture. I would also like to thank larowlan who put a tonne of work into this patch so we can get it over the line.

Upon inspecting with the merged patch #94 I noticed that the last tip isn't being provided the "End tour" string (it still has "Next").

YesCT’s picture

#1917212: Add checkbox in installer to enable content translation (if in foreign language) is watching this issue to see if this can help there.

Should we (I) open a separate issue to work on what the tour will actually be, so this can just implement a proof of concept, and we work out the whole script in another issue?

effulgentsia’s picture

Priority: Normal » Major

Since this can help with #1164760: [meta] New users universally did NOT understand that you can extend Drupal, bumping it up to major. Great work everyone. Looking forward to seeing this get wrapped up and land.

tim.plunkett’s picture

I love the new architecture.
I'll give it a day for any other reviews, but I think this is RTBC.

nick_schuch’s picture

As per #104 I have fixed the "End tour" functionality.

I believe we are down to reviews.

Status: Needs review » Needs work
Issue tags: -JavaScript, -Usability, -Needs manual testing, -Needs accessibility review

The last submitted patch, tour-module-1809352.108.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: +JavaScript, +Usability, +Needs manual testing, +Needs accessibility review

#108: tour-module-1809352.108.patch queued for re-testing.

jthorson’s picture

Whitespace fix.

Status: Needs review » Needs work

The last submitted patch, tour-module-1809352.111.patch, failed testing.

jthorson’s picture

Status: Needs work » Needs review
FileSize
1.39 KB

Test requeued on bot. (bot problem, not patch)

And because you asked so nicely ... ;)

YesCT’s picture

lovely! :)

Wim Leers’s picture

FileSize
11.61 KB
79.14 KB

First impressions (of #104/#111)

Code is much nicer :)

The included tips are no longer those for the "Search" block and the admin/content page, but instead, now there is only a whole series of tips for the "edit view" form.

Changes

  • Fixed a bunch of extremely nitpicky, extremely superduper minor whitespace "issues".
  • Fixed some docs.
  • Improved the example tips: changed their weights from strings to integers, fixed language problems.
  • After installing the module, I saw the "Tour" tab on the admin/modules page … because there's a <label id="tour" />, which matches the #tour selector… — my bad! I fixed it by making the selector a tad more specific.

Review

+++ b/core/modules/tour/lib/Drupal/tour/Plugin/Core/Entity/Tour.phpundefined
@@ -0,0 +1,136 @@
+ * Defines the configured text tour entity.

"text tour" -> then why don't the label and the class name confirm this specificity?
All other aspects of this class suggest that this is the most generic tour possible.

+++ b/core/modules/tour/lib/Drupal/tour/TourManager.phpundefined
@@ -0,0 +1,51 @@
+ * Configurable text tour manager.
+ */
+class TourManager extends PluginManagerBase {

Tour manager or *text* tour manager?

(Sh|W)ouldn't this affect *all* tours?

+++ b/core/modules/tour/tour.moduleundefined
@@ -0,0 +1,223 @@
+/**
+ * Implements hook_preprocess_HOOK() for page.tpl.php.
+ */
+function tour_preprocess_page(&$variables) {
+  $path = current_path();
+  $langcode = language(LANGUAGE_TYPE_CONTENT)->langcode;
+  $cid = $path . ':' . $langcode;
+  $cache = cache('cache_tour')->get($cid);
+  $tour_items = array();
+  if ($cache) {
+    // Cache hit.
+    $tour_items = $cache->data;
+  }
+  else {
…
+    // Cache for future requests.
+    cache('cache_tour')->set($cid, $tour_items, CacheBackendInterface::CACHE_PERMANENT, array('tour_items'));
+  }

This… is highly problematic. It creates a *permanent* cache entry for *every* (path, langcode) pair possible on a Drupal site. Clearly, this will grow ridiculously fast. Do we really need this caching? Do you have the numbers to back it up?

Why is this code executing unconditionally, i.e. even if the current user does not have access to tours?

Finally, I don't know the cache API well and its conventions not at all, but it strikes me as odd that you create a new cache bin and then tag all of the cache entries in the same way. Better not use a cache tag in that case? Or, better, use the langcode as a tag, which seems sensible (i.e. after importing new translations, one could then clear the cache of tour tips of just that language).

larowlan’s picture

Comments on discussion from irc with @WimLeers about #115.

+ * Defines the configured text tour entity.
Yes the term 'text' should go.

+ * Configurable text tour manager.
Same

+/**
+ * Implements hook_preprocess_HOOK() for page.tpl.php.
+ */
+function tour_preprocess_page(&$variables) {
+  $path = current_path();
+  $langcode = language(LANGUAGE_TYPE_CONTENT)->langcode;
+  $cid = $path . ':' . $langcode;
+  $cache = cache('cache_tour')->get($cid);
+  $tour_items = array();
+  if ($cache) {
+    // Cache hit.
+    $tour_items = $cache->data;
+  }
+  else {
…
+    // Cache for future requests.
+    cache('cache_tour')->set($cid, $tour_items, CacheBackendInterface::CACHE_PERMANENT, array('tour_items'));
+  }

1) We should wrap this in user_access('access tour')
2) We should indicate that this is a stop gap solution and should be removed once #1918768: Refactor tour module to use routes instead of paths is in.

nick_schuch’s picture

The following patch resolves the items listed in #116.

I would also like to put my hand up to maintain this module. I have also reinstated the change to MAINTAINERS.txt which had me down for this module (I believe I accidentally removed it in one of my earlier commits).

nick_schuch’s picture

Fixed my sentence structure.

nick_schuch’s picture

Fixed my sentence structure.

tim.plunkett’s picture

Last nitpicks before RTBC, I think.

+++ b/core/modules/tour/lib/Drupal/tour/Plugin/tour/tip/TipPluginText.phpundefined
@@ -0,0 +1,96 @@
+      '#markup' => '<h2 class="tour-tip-label" id="tour-tip-' . $this->getAriaId() . '-label">' . check_plain($this->getLabel()) . '</h2>
+      <p class="tour-tip-body" id="tour-tip-' . $this->getAriaId() . '-contents">' . filter_xss_admin($this->getBody()) . '</p>'

+++ b/core/modules/tour/tests/tour_test/lib/Drupal/tour_test/Plugin/tour/tip/TipPluginImage.phpundefined
@@ -0,0 +1,48 @@
+      '#markup' => '<h2 class="tour-tip-label" id="tour-tip-' . $this->get('ariaId') . '-label">' . check_plain($this->get('label')) . '</h2>
+      <p class="tour-tip-image" id="tour-tip-' . $this->get('ariaId') . '-contents">' . theme('image', array('uri' => $this->get('url'), 'alt' => $this->get('alt'))) . '</p>'

These are weirdly split up. maybe a temp variable that gets added to?

$output = ''; $output .= '<h2...';
$output .= '<p...';
$output .= '</h2>';

or something

+++ b/core/modules/tour/lib/Drupal/tour/Tests/TourPluginTest.phpundefined
@@ -0,0 +1,59 @@
+  /**
+   * Defines test info.
+   */
+  public static function getInfo() {
...
+  /**
+   * Sets up the test.
+   */
+  protected function setUp() {

No comments needed on these methods

+++ b/core/modules/tour/lib/Drupal/tour/Tests/TourTest.phpundefined
@@ -0,0 +1,149 @@
+   * Test tour functionality.

Tests

+++ b/core/modules/tour/lib/Drupal/tour/Tests/TourTest.phpundefined
@@ -0,0 +1,149 @@
+    $elements = $this->xpath('//li[@data-id=:data_id and ./h2[contains(., :text)]]', array(
+      ':data_id' => 'tour-test-1',
+      ':text' => 'The first tip'

Here, and everywhere in the method, missing trailing comma

+++ b/core/modules/tour/lib/Drupal/tour/Tests/TourTest.phpundefined
@@ -0,0 +1,149 @@
+  }
+}
diff --git a/core/modules/tour/lib/Drupal/tour/TipPluginBase.php b/core/modules/tour/lib/Drupal/tour/TipPluginBase.php

diff --git a/core/modules/tour/lib/Drupal/tour/TipPluginBase.php b/core/modules/tour/lib/Drupal/tour/TipPluginBase.php
new file mode 100644

Missing blank line

+++ b/core/modules/tour/lib/Drupal/tour/TipPluginBase.phpundefined
@@ -0,0 +1,89 @@
+    parent::__construct($configuration, $plugin_id, $discovery);
+    $this->definition = $this->discovery->getDefinition($plugin_id);

Blank line would be nice

+++ b/core/modules/tour/lib/Drupal/tour/TourManager.phpundefined
@@ -0,0 +1,52 @@
+  /**
+   * Overrides \Drupal\Component\Plugin\PluginManagerBase::createInstance().
+   */
+  public function createInstance($plugin_id, array $configuration = array(), TipsBag $bag = NULL) {
+    $plugin_class = DefaultFactory::getPluginClass($plugin_id, $this->discovery);
+    return new $plugin_class($configuration, $plugin_id, $this->discovery, $bag);

This can be completely removed.

+++ b/core/modules/tour/tests/tour_test/lib/Drupal/tour_test/Plugin/tour/tip/TipPluginImage.phpundefined
@@ -0,0 +1,48 @@
+  }
+}
diff --git a/core/modules/tour/tests/tour_test/tour_test.info b/core/modules/tour/tests/tour_test/tour_test.info

Missing blank line

+++ b/core/modules/tour/tour.moduleundefined
@@ -0,0 +1,223 @@
+ * Implements hook_tour_tour_insert().
...
+function tour_tour_insert($entity) {
...
+ * Implements hook_tour_tour_update().
...
+function tour_tour_update($entity) {

Guessing they would just bee hook_tour_insert() and hook_tour_update()

YesCT’s picture

Here is the link that shows no function doc needed for test setUp, getinfo. Also should be public, not protected:
http://drupal.org/node/325974

nick_schuch’s picture

Here are the fixes identified in #120.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing

Now we all need is a Tour Admin UI! :)

I've manually tested this at each step, reviewed the architecture, and nitpicked it to death. Ship it!

tsvenson’s picture

Assigned: nick_schuch » tsvenson
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs manual testing

A lot of amazing work has been done with the functionality, look and feel of this patch since the last UX review was done.

So, for that reason I'm assigning to myself and changing status to needs review as I'm going to make a new UX review when I get back home in about 5h or so.

Gábor Hojtsy’s picture

Assigned: tsvenson » Unassigned
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing

Discussed with @tsvenson. He is not likely to have time to review this immediately (also as said above), but don't want this blocked from being committed, so moving back to RTBC. Also remove manual testing tab as per above (#123).

Dries’s picture

As mention at DrupalCon Sydney, I love this module. We should move forward with it.

We should consider deprecating the existing help pages (the ones at ?q=admin/help) in favor of this module and possibly some of the inline help as well. We can discuss this in a separate issue.

I think that most regular users would refer to this as 'help' (instead of 'tour') but we can discuss this later as well. If someone needs help, they are most likely to search for the word 'help' or '?', and not for the word 'tour'.

LewisNyman’s picture

I agree with Dries. The context is even stronger on a mobile device, where users only see icons in the toolbar, it is not clear whether the ? icon takes you to the help page or does something else.

I also think the ultimate fate of this module is to expand (eg. on ramp) and become flexible enough to replace the help section entirely.

Dries’s picture

#122: tour_module-1809352-121.patch queued for re-testing.

Dries’s picture

The patch may need a quick re-roll. Doesn't seem to apply on my local host (but seemed to work earlier on simplytest.me).

yoroy’s picture

Very happy to see this got so much attention over the last couple of weeks. It looks like people agree that this lays a solid foundation. I'm pretty sure that means everything else can be captured in followups :-)
+1 on RTBC.

vijaycs85’s picture

#122 seem to be "git apply --index" OK in my local.

mgifford’s picture

@Dries - tour_module-1809352-121.patch applied nicely on my local system. Not sure.

tsvenson’s picture

Assigned: nick_schuch » Unassigned
Priority: Normal » Major
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing +Needs accessibility review

@Dries:

We should consider deprecating the existing help pages (the ones at ?q=admin/help) in favor of this module and possibly some of the inline help as well. We can discuss this in a separate issue.

I think that most regular users would refer to this as 'help' (instead of 'tour') but we can discuss this later as well. If someone needs help, they are most likely to search for the word 'help' or '?', and not for the word 'tour'.

I outlined a few ideas about how this module can be used both for the original use - Giving a guided tour, and also for inline help similar, but better, to how the Advanced Help module does it for Views.

See #85 for more info about that.

With doing so it would also be quite easy to have two different themes, one for the tour and one for the inline help to make it easier for users to distinguish between them.

vijaycs85’s picture

FileSize
35.54 KB

#129 is right. I don't see "tour" any more in toolbar after installation (with patch in #122).
2013-02-18_205925.png

LewisNyman’s picture

@vijaycs85 you do once you land on a views edit screen.

vijaycs85’s picture

Got it :) thanks @LewisNyman.

jessebeach’s picture

The patch in #122 is missing jQuery.once() around the Drupal.attach setup code.

Wim Leers’s picture

Great catch, Jesse, thanks!

Dries’s picture

Like @vijaycs85 in #134, I was confused about not seeing the tour icon after applying the patch. I actually had to look at the code to figure out how it worked, and I had to look at the YAML files to figure out where a tour icon would appear. I wonder if it would be better to always show the help/tour-icon, even if no help is available. If no help is available, the (1) icon could have a different color and (2) say "No help is available for this page" when clicked. I think it would help with the discovery and learnability of this feature. For what it is worth, I had the same issues with the 'Edit' toggle for in-place editing. I personally prefer to have permanent icons (both for help mode and edit mode) that are explicit about the features not being available on specific pages, than having them disappear/appear randomly. Not sure if most people feel that way though; my opinion may not be representative for the larger audience. Worth discussing more, but not a showstopper at this point.

YesCT’s picture

FileSize
130.36 KB

I think some of that might go away as the script for the tour is expanded to cover more/ another script is added to tour drupal itself, like pointing out the extend. (which I think we still need a follow-up for)

Also, I think that discussing replacing help or advanced help is going to be done in a follow-up.

Maybe have a link to the tour(s) from the Help page permanently? (can it even work like that?)

here is a recent screenshot (which serves as a visual reminder for people coming to review that it shows when editing a view).
2013-02-18_1223.png

effulgentsia’s picture

YesCT’s picture

I updated the issue summary to add a follow-up section, if I missed any follow-ups, please add them or comment on what is needed.

Also, we might want to add a tour component or repurpose another component (which one, help?)

YesCT’s picture

Issue summary: View changes

added follow-up section and some follow-ups

Berdir’s picture

Status: Reviewed & tested by the community » Needs review

I was going to comment on the cache() usage in the patch and then noticed #115 already covers most what I wanted to say.

Yes, it makes no sense to use both cache tags and custom cache bin with nothing else in it. Just use deleteAll() to empty the bin.

And as has been said, I'm not sure that whole cache handling is really necessary, looks like premature optimization to me. I'd vote for removing the cache handling and the whole cache_tour bin and maybe check later how many tips we actually end up having and if it's worth to add some caching.

If you need to get it in and ready today, that can of course be done as a follow-up but it shouldn't be too hard to remove it.

larowlan’s picture

Assigned: Unassigned » larowlan

Removing caching

effulgentsia’s picture

Assigned: larowlan » Unassigned
Status: Needs review » Reviewed & tested by the community

If you need to get it in and ready today, that can of course be done as a follow-up but it shouldn't be too hard to remove it.

Restoring this to RTBC then, to keep this in front of committers. If someone wants to implement #144 before commit, please don't let this status stop you.

effulgentsia’s picture

Assigned: Unassigned » larowlan
larowlan’s picture

Assigned: larowlan » Unassigned
FileSize
76.34 KB
3.43 KB

New patch removes caching
Knocking back to RTBC if bot comes back green.

Bojhan’s picture

I agree with @Dries, about discover-ability. I am not sure if the solution is a persistent button in the toolbar. Especially since a tour option, is a initial go-to-place, not something people will often reuse after, even if you consider intermediates they will quickly avoid tours, its in their nature :)

This should get a structured approach, and much more UX thought on how its displayed, when its displayed and where it fits in the larger picture of learning Drupal (we can't stuff the admin UI with tours, everywhere when people first install Drupal). This structured UX approach has not happened, the focus has been on getting the feature to core level in terms of code. Its up to the commiters to evaluate, whether we can improve this further in core or not.

Dries’s picture

Here is how I look at the patch: the most valuable part of this patch is the opportunity to add detailed, highly-contextual help texts. The way these "tooltips" are presented is both beautiful and it allows for a fair amount of detail. Great! The fact that these "tooltips" are presented as tour is certainly helpful, but secondary. For example, I could imagine a mode where I could jump to the element of the page where I need help (versus having to go through a tour). Hence, why I feel that this provides an opportunity to rethink the help system as a whole.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

While we have a lot of thinking to do about how to deliver the best possible help system, I have no doubt that that the tour module adds 'strong bones', so to speak. I've decided to commit the tour module before feature freeze so we can work with it post feature freeze. If necessary, we can rename the module post feature freeze.

Great job everyone. This is exciting. :)

jessebeach’s picture

Would be great to get a change notice about this. I'll leave this to the major contributors so you get a chance to bask a bit in a major improvement.

http://drupal.org/node/add/changenotice

larowlan’s picture

Title: Write tour.module and add it to core » Change Notice: Write tour.module and add it to core
Priority: Major » Critical
Status: Fixed » Active
Issue tags: +Needs change record

Yep, change notice en-route

nick_schuch’s picture

There was a miscommunication between myself and larowlan. He believed that the latest was pushed up to the sandbox.

Here is a patch to add all the fixes from #117 onwards.

nick_schuch’s picture

Status: Active » Needs review
jessebeach’s picture

Status: Needs review » Active

No worries. Diffing and testing now.

jessebeach’s picture

Status: Active » Reviewed & tested by the community

The changes looks fine. Just some comment cleanup, minor refactoring, and the jQuery.once() that I introduced in #137.

cweagans’s picture

To clarify, is it just that 148 needs rolled back and 154 needs to be applied instead?

nick_schuch’s picture

cweagans, it this patch just goes on top of the current implementation.

webchick’s picture

Status: Reviewed & tested by the community » Active

Hasn't reported back yet, but http://qa.drupal.org/pifr/test/448873 shows this patch passing testbot.

Committed and pushed to 8.x. Thanks!

Back to active for the change notice.

mgifford’s picture

Issue tags: -Usability

Nice, this is a great feature and some terrific work went into this patch!

EDIT: No idea how the notice got removed. I didn't do it intentionally, but don't think it matters now either.

jthorson’s picture

FileSize
1.39 KB

Really really trivial tweak ...

andypost’s picture

And the tour module needs hook_help() to be deprecated latter :)

nick_schuch’s picture

Status: Active » Needs review
FileSize
1.85 KB
468 bytes

Update for CHANGELOG.txt to include:

"Added tour module. Provides highly contextual tips for UI elements."

jibran’s picture

Can we add to change notice which pages are having tour option?

nick_schuch’s picture

jibran, I have created the following:

http://drupal.org/node/1921162

jibran’s picture

Title: Change Notice: Write tour.module and add it to core » Write tour.module and add it to core
Component: other » tour.module
Priority: Critical » Major

Change notice looks good.

tim.plunkett’s picture

Status: Needs review » Fixed
Issue tags: -Needs accessibility review, -Needs change record
jibran’s picture

Category: feature » task
Priority: Major » Normal
Status: Fixed » Needs review

It is CNR because of #164. Still a patch pending to commit.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Last patch looks good.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

nick_schuch’s picture

Status: Fixed » Closed (fixed)

Ok, we can close this one and handle all future work in follow ups.

effulgentsia’s picture

Category: task » feature
Priority: Normal » Major
Status: Closed (fixed) » Fixed

Our normal process is to leave fixed issues as regular "fixed", for easier discoverability while it's still new. d.o. will automatically move it to "closed (fixed)" after 2 weeks of no comments.

Also restoring original category and priority for d.o. statistics.

nick_schuch’s picture

Sorry effulgentsia. Will keep this in mind moving forward.

mgifford’s picture

Status: Fixed » Active

The Tour link moved since the last time I used it.

I had to re-open this though as it doesn't seem to be accessible without a mouse. At least there is not :focus state which would indicate to you that you are hovering on-top of the link.

This could be addressed in another issue, but wanted to see it wasn't forgotten.

larowlan’s picture

Status: Active » Fixed

Hey mgifford, the link was changed to a button so we just might need some :focus css
Added #1924112: Make sure tour toolbar button has :focus styling when tabbed to. and update issue summary

larowlan’s picture

Issue summary: View changes

Updated issue summary, with template and steps to test.

yched’s picture

yched’s picture

Issue summary: View changes

Updated issue summary.

larowlan’s picture

larowlan’s picture

Issue summary: View changes

added follow-up to the issue summary

larowlan’s picture

Issue summary: View changes

Updated issue summary.

larowlan’s picture

LeeHunter’s picture

Issue tags: +docs infrastructure

Adding docs infrastructure tag

Gábor Hojtsy’s picture

I'm pretty puzzled by the tour module's language use. It is pretty non-standard. Can someone enlighten me in #1935120: Unusual language use in tour module? Thanks a lot!

larowlan’s picture

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.