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
- git pull to get most recent drupal 8
- apply most recent patch
- install
- add a view and edit it
User interface changes
API changes
Needs to be described.
Follow-ups
- #1920468: Write tour integration for the first page after install showing extend and other things
- #1920470: Find out how help and tour can work together
- #1920462: Show (disabled) Tour and Edit buttons on pages with no tour/edit available
- #1924112: Make sure tour toolbar button has :focus styling when tabbed to.
- #1925576: "Tip" plugins managed by TourManager ??
- #1921152: META: Start providing tour tips for other core modules.
- #1932048: Clean up tour ConfigEntity architecture
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.
Comment | File | Size | Author |
---|---|---|---|
#164 | interdiff.txt | 468 bytes | nick_schuch |
#164 | 1809352-163.patch | 1.85 KB | nick_schuch |
#162 | 1809352-162.patch | 1.39 KB | jthorson |
#154 | tour_module-1809352-154.patch | 14.79 KB | nick_schuch |
#148 | tour-module-1809352.146.interdiff.txt | 3.43 KB | larowlan |
Comments
Comment #1
larowlan+1, we were talking about a slideshow while the installer ran at one point too, like you get when you install Ubuntu/Fedora
Comment #2
tim.plunkettThat joyride library is pretty awesome. +1
Comment #3
Devin Carlson CreditAttribution: Devin Carlson commentedI 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:
Installation
Notes
Comment #4
webchickThere 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.
Comment #6
cweagansWow, 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?
Comment #7
cweagansWhoops.
Comment #8
Grayside CreditAttribution: Grayside commentedDepending 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.
Comment #9
yoroy CreditAttribution: yoroy commentedWhat 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.
Comment #10
Fabianx CreditAttribution: Fabianx commented#3: add-tour-module-1809352-3.patch queued for re-testing.
Comment #11
Fabianx CreditAttribution: Fabianx commentedHuge +1 for this.
What is missing for RTBC besides a re-roll probably?
Comment #12
cweagansI 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.
Comment #13
cweagansI 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).
Comment #14
Bojhan CreditAttribution: Bojhan commentedI 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.
Comment #15
jenlamptonFor 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.
thenDrupal 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"?
Comment #16
cweagansThere 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.
Comment #17
Fabianx CreditAttribution: Fabianx commentedI 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? :-)
Comment #18
Fabianx CreditAttribution: Fabianx commented#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!
Comment #19
nick_schuch CreditAttribution: nick_schuch commentedI have worked on a plugin solution over the past few days. I will post plans and details of this work very shortly.
Comment #20
nick_schuch CreditAttribution: nick_schuch commentedThe 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:
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).
Comment #21
nick_schuch CreditAttribution: nick_schuch commentedImages (cannot use via external service).
Comment #22
cweagansI 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?)
Comment #23
tim.plunkettThe first line will be translated. The second will not, but could be if it was wrapped in @Translation
Comment #24
cweagansWould 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?
Comment #25
nick_schuch CreditAttribution: nick_schuch commentedThanks 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.
Comment #26
markwk CreditAttribution: markwk commentedCool to see this discussion for core. Here is my contrib route http://drupal.org/project/joyride
Comment #27
larowlanSo 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
Comment #29
jessebeach CreditAttribution: jessebeach commentednick_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.
Comment #30
larowlanNext patch will need review of the JavaScript
Have reached out to a11y group for their input into the best markup.
Tagging accordingly
Comment #31
nod_Missing dependencies in the library_info hook
More readable JS with some new core things added (and the behavior name was wrong):
Comment #32
jessebeach CreditAttribution: jessebeach commentedIt 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:
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.
Comment #33
nick_schuch CreditAttribution: nick_schuch commentedHi jbeach!
Myself and larowlan are currently working out of a branch in a sandbox. Will have a patch for review very soon.
Comment #34
larowlanThis 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)
Comment #35
mgiffordI 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.
Comment #36
tsvenson CreditAttribution: tsvenson commented@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)".
Comment #37
larowlanthanks @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++
Comment #38
mgiffordI 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.
Comment #39
larowlanLatest 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
Comment #40
tsvenson CreditAttribution: tsvenson commented@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:
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.
Comment #41
jessebeach CreditAttribution: jessebeach commentedThe joyride plugin is setting
position:relative
on thebody
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.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.
Comment #42
jessebeach CreditAttribution: jessebeach commentedThis 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.
Comment #43
jessebeach CreditAttribution: jessebeach commentedOh, 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.
Comment #44
effulgentsia CreditAttribution: effulgentsia commentedFor anyone playing along at home, here's an up to date patch from the latest commit in the sandbox, and interdiff relative to #34.
Comment #45
larowlanThanks @effulgentsia, to describe the latest patch - this provides
Can we make sure @'Christian Biggins' gets some commit credit here as he's spent at least an hour with me looking at this.
Comment #46
effulgentsia CreditAttribution: effulgentsia commentedThis 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?
Comment #47
effulgentsia CreditAttribution: effulgentsia commented#44: tour-module-1809352.44.patch queued for re-testing.
Comment #48
effulgentsia CreditAttribution: effulgentsia commentedI'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.
Comment #49
Ralt CreditAttribution: Ralt commentedHi,
I made a little review of the JS part.
console.log
should be removed from production.$(toolbar, scope)
is used several times. It should be stored somewhere.$(items + " li")
should be$("li", items)
. It's also used several times, so subject to caching.$(this).data('text')
should be used instead of$(this).attr('data-text')
. Same for$(this).attr('data-id')
and$(this).attr('data-class')
.$(items).attr('hidden', true)
should be$(items).prop('hidden', true)
. Also, thehidden
property should be used with care, since it can be overriden bydisplay: block;
.Besides, shouldn't simple quotes/double quotes usage be unified?
detachTour()
andsetupTour()
are used twice together. Shouldn't there be arestartTour()
method?Regards,
Florian
Comment #50
Ralt CreditAttribution: Ralt commentedComment #51
Wim LeersVery, 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 :)
s/function(context)/function (context)/
See http://drupal.org/node/172169.
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.s/function( index )/function (index)/
See http://drupal.org/node/172169.
s/Close/End tour/
console.log()
s should be removed.s/Naviate/Navigate/
Newline between these.
Newline between these.
Missing leading backslash.
Trailing space.
s/api/API/
Should document the parameters.
Trailing space; should contain sample code.
This comment can go away.
Instead of calling
drupal_get_path()
many, many times, call it once, store it in a variable, reuse that.type = file
is the default, no need to specify that here.This comment can go away.
These comments can also go away.
Why can't you use
theme_item_list()
?You're already attaching the library in
hook_toolbar()
, why is it then also unconditionally loaded viahook_page_build()
?Comment #52
larowlanHi 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
Comment #53
tim.plunkettThis 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.
This creates an instance of every tour plugin, always.
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.
Missing package
Hook docblocks are declarative, so "Allow modules..". It should go on to explain why/how to do this, and also document the parameters
I have no idea why you would want or need to alter these, and this should give an example.
Where is this invoked?
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.
use '#theme' => 'tour_items', not '#markup'
Comment #54
effulgentsia CreditAttribution: effulgentsia commentedThat 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.
Comment #55
CBI've spent a bit of time going over this from an accessibility viewpoint this morning, I'm not done yet but some notes;
Super nice module though. Double thumbs up.
Comment #56
Grayside CreditAttribution: Grayside commentedI 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.
Comment #57
effulgentsia CreditAttribution: effulgentsia commentedIn 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?
Comment #58
effulgentsia CreditAttribution: effulgentsia commentedAlso, 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?
Comment #59
tsvenson CreditAttribution: tsvenson commentedI 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:
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:
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.
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:
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.
Comment #60
tsvenson CreditAttribution: tsvenson commentedCan this be used on live sites too?
When I do a git status I see lines like:
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.
Comment #61
nick_schuch CreditAttribution: nick_schuch commentedFirst 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!
Comment #62
sunI 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!
Comment #63
CBSome ideas for icons;
Camera is probably my pick.
Comment #64
nick_schuch CreditAttribution: nick_schuch commentedThanks 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).
Comment #65
jwilson3As 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.
Comment #66
larowlanHi all, I think this one
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.
Comment #67
larowlanThanks Nick for your work on this.
Saw some whitespace issues, listing them while I think of it.
whitespace
whitespace
whitespace
whitespace
whitespace
Comment #68
jwilson3Also, visible from the review in #67: prefix variables that point to jQuery objects with a dollar sign.
Comment #69
nick_schuch CreditAttribution: nick_schuch commentedThis resolves the issues in #67 and #68.
Comment #70
mgiffordTested 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?
Comment #71
nick_schuch CreditAttribution: nick_schuch commentedThanks mgifford!
I believe im up to the last item as listed in #61 which relates to this:
Im currently in IRC with tim.plunkett trying to get it resolved.
Comment #72
tim.plunkettBecause 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.
Comment #73
CBNVDA reads the dialogs correctly still using IE (but not Chrome due to how it handles Aria - see bug)
Comment #74
Grayside CreditAttribution: Grayside commented@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.
Comment #75
nick_schuch CreditAttribution: nick_schuch commentedWhat 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.
Comment #76
larowlanI 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:
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.
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' ?
This is an MIT license, we need to make sure that it is ok for this to be included in a GPL project.
Comment #77
larowlanjudging by http://www.gnu.org/licenses/license-list.html#GPLCompatibleLicenses and http://drupal.org/licensing/faq, MIT license is ok.
Comment #78
larowlanremoving the position:relative from the body element from the joyride-2.03.css resolves the positioning issues.
Comment #79
larowlanFixes 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.
Comment #80
larowlanMeh, wrong patch file
Comment #81
tsvenson CreditAttribution: tsvenson commentedTo 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:
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:
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.
Comment #82
larowlan@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.
Comment #83
tsvenson CreditAttribution: tsvenson commented@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.
Comment #84
larowlanI 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
Comment #85
tsvenson CreditAttribution: tsvenson commentedProposal 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:
Those folders can live as subfolders to for example:
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.
Comment #86
cweagans-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.
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.
Comment #87
Gábor Hojtsy@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.
Comment #88
tsvenson CreditAttribution: tsvenson commented@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.
Comment #89
Gábor Hojtsy@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.
Comment #90
Wim LeersI'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.
Comment #91
tim.plunkett@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.
Comment #92
Wim LeersI 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.)Comment #93
tim.plunkett@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:
Comment #94
Wim LeersChanges in rerolled patch
.joyride-tip-guide { z-index: 502; }
was pointless, because it was being overridden byjoyride-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..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.TODOs/observations
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()
andhook_tour_tooltip_update()
are not documented intour.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!data-text
? Why can't we just always generate "Next" and "End tour" for the last item?Comment #95
larowlanThanks @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.
Comment #96
Wim Leers#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 :)
Comment #97
larowlanAs 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 :)
Comment #98
Wim LeersWhat's the status on the PHP side (as per #91 & #93)? What can I do to help this move forward? :)
Comment #99
larowlanHi 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
Comment #100
nick_schuch CreditAttribution: nick_schuch commentedI 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
Comment #101
Wim Leers#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.)
Comment #102
nick_schuch CreditAttribution: nick_schuch commentedRight you are Wim Leers.
We are now down to refactoring of tests in our branch. Myself and larowlan are both looking at these.
Comment #103
nick_schuch CreditAttribution: nick_schuch commentedI 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".
Comment #104
nick_schuch CreditAttribution: nick_schuch commentedOk, 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").
Comment #105
YesCT CreditAttribution: YesCT commented#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?
Comment #106
effulgentsia CreditAttribution: effulgentsia commentedSince 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.
Comment #107
tim.plunkettI love the new architecture.
I'll give it a day for any other reviews, but I think this is RTBC.
Comment #108
nick_schuch CreditAttribution: nick_schuch commentedAs per #104 I have fixed the "End tour" functionality.
I believe we are down to reviews.
Comment #110
Wim Leers#108: tour-module-1809352.108.patch queued for re-testing.
Comment #111
jthorson CreditAttribution: jthorson commentedWhitespace fix.
Comment #113
jthorson CreditAttribution: jthorson commentedTest requeued on bot. (bot problem, not patch)
And because you asked so nicely ... ;)
Comment #114
YesCT CreditAttribution: YesCT commentedlovely! :)
Comment #115
Wim LeersFirst 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
<label id="tour" />
, which matches the#tour
selector… — my bad! I fixed it by making the selector a tad more specific.Review
"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.
Tour manager or *text* tour manager?
(Sh|W)ouldn't this affect *all* tours?
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).
Comment #116
larowlanComments 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
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.
Comment #117
nick_schuch CreditAttribution: nick_schuch commentedThe 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).
Comment #118
nick_schuch CreditAttribution: nick_schuch commentedFixed my sentence structure.
Comment #119
nick_schuch CreditAttribution: nick_schuch commentedFixed my sentence structure.
Comment #120
tim.plunkettLast nitpicks before RTBC, I think.
These are weirdly split up. maybe a temp variable that gets added to?
or something
No comments needed on these methods
Tests
Here, and everywhere in the method, missing trailing comma
Missing blank line
Blank line would be nice
This can be completely removed.
Missing blank line
Guessing they would just bee hook_tour_insert() and hook_tour_update()
Comment #121
YesCT CreditAttribution: YesCT commentedHere is the link that shows no function doc needed for test setUp, getinfo. Also should be public, not protected:
http://drupal.org/node/325974
Comment #122
nick_schuch CreditAttribution: nick_schuch commentedHere are the fixes identified in #120.
Comment #123
tim.plunkettNow 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!
Comment #124
tsvenson CreditAttribution: tsvenson commentedA 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.
Comment #125
Gábor HojtsyDiscussed 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).
Comment #126
Dries CreditAttribution: Dries commentedAs 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'.
Comment #127
LewisNymanI 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.
Comment #128
Dries CreditAttribution: Dries commented#122: tour_module-1809352-121.patch queued for re-testing.
Comment #129
Dries CreditAttribution: Dries commentedThe patch may need a quick re-roll. Doesn't seem to apply on my local host (but seemed to work earlier on simplytest.me).
Comment #130
yoroy CreditAttribution: yoroy commentedVery 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.
Comment #131
vijaycs85#122 seem to be "git apply --index" OK in my local.
Comment #132
mgifford@Dries - tour_module-1809352-121.patch applied nicely on my local system. Not sure.
Comment #133
tsvenson CreditAttribution: tsvenson commented@Dries:
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.
Comment #134
vijaycs85#129 is right. I don't see "tour" any more in toolbar after installation (with patch in #122).
Comment #135
LewisNyman@vijaycs85 you do once you land on a views edit screen.
Comment #136
vijaycs85Got it :) thanks @LewisNyman.
Comment #137
jessebeach CreditAttribution: jessebeach commentedThe patch in #122 is missing
jQuery.once()
around theDrupal.attach
setup code.Comment #138
Wim LeersGreat catch, Jesse, thanks!
Comment #139
Dries CreditAttribution: Dries commentedLike @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.
Comment #140
YesCT CreditAttribution: YesCT commentedI 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).
Comment #141
effulgentsia CreditAttribution: effulgentsia commentedFollow up for #139: #1920462: Show (disabled) Tour and Edit buttons on pages with no tour/edit available
Comment #142
YesCT CreditAttribution: YesCT commentedFollow-ups
#1920468: Write tour integration for the first page after install showing extend and other things
#1920470: Find out how help and tour can work together
Comment #143
YesCT CreditAttribution: YesCT commentedI 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?)
Comment #143.0
YesCT CreditAttribution: YesCT commentedadded follow-up section and some follow-ups
Comment #144
BerdirI 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.
Comment #145
larowlanRemoving caching
Comment #146
effulgentsia CreditAttribution: effulgentsia commentedRestoring 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.
Comment #147
effulgentsia CreditAttribution: effulgentsia commentedComment #148
larowlanNew patch removes caching
Knocking back to RTBC if bot comes back green.
Comment #149
Bojhan CreditAttribution: Bojhan commentedI 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.
Comment #150
Dries CreditAttribution: Dries commentedHere 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.
Comment #151
Dries CreditAttribution: Dries commentedWhile 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. :)
Comment #152
jessebeach CreditAttribution: jessebeach commentedWould 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
Comment #153
larowlanYep, change notice en-route
Comment #154
nick_schuch CreditAttribution: nick_schuch commentedThere 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.
Comment #155
nick_schuch CreditAttribution: nick_schuch commentedComment #156
jessebeach CreditAttribution: jessebeach commentedNo worries. Diffing and testing now.
Comment #157
jessebeach CreditAttribution: jessebeach commentedThe changes looks fine. Just some comment cleanup, minor refactoring, and the
jQuery.once()
that I introduced in #137.Comment #158
cweagansTo clarify, is it just that 148 needs rolled back and 154 needs to be applied instead?
Comment #159
nick_schuch CreditAttribution: nick_schuch commentedcweagans, it this patch just goes on top of the current implementation.
Comment #160
webchickHasn'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.
Comment #161
mgiffordNice, 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.
Comment #162
jthorson CreditAttribution: jthorson commentedReally really trivial tweak ...
Comment #163
andypostAnd the tour module needs hook_help() to be deprecated latter :)
Comment #164
nick_schuch CreditAttribution: nick_schuch commentedUpdate for CHANGELOG.txt to include:
"Added tour module. Provides highly contextual tips for UI elements."
Comment #165
jibranCan we add to change notice which pages are having tour option?
Comment #166
nick_schuch CreditAttribution: nick_schuch commentedjibran, I have created the following:
http://drupal.org/node/1921162
Comment #167
jibranChange notice looks good.
Comment #168
tim.plunkettComment #169
jibranIt is CNR because of #164. Still a patch pending to commit.
Comment #170
larowlanLast patch looks good.
Comment #171
webchickCommitted and pushed to 8.x. Thanks!
Comment #172
nick_schuch CreditAttribution: nick_schuch commentedOk, we can close this one and handle all future work in follow ups.
Comment #173
effulgentsia CreditAttribution: effulgentsia commentedOur 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.
Comment #174
nick_schuch CreditAttribution: nick_schuch commentedSorry effulgentsia. Will keep this in mind moving forward.
Comment #175
mgiffordThe 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.
Comment #176
larowlanHey 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
Comment #176.0
larowlanUpdated issue summary, with template and steps to test.
Comment #177
yched CreditAttribution: yched commentedFWIW: #1925576: "Tip" plugins managed by TourManager ??
Comment #177.0
yched CreditAttribution: yched commentedUpdated issue summary.
Comment #178
larowlanAdded #1921152: META: Start providing tour tips for other core modules. to summary
Comment #178.0
larowlanadded follow-up to the issue summary
Comment #178.1
larowlanUpdated issue summary.
Comment #179
larowlanAdded #1932048: Clean up tour ConfigEntity architecture to summary
Comment #180
LeeHunter CreditAttribution: LeeHunter commentedAdding docs infrastructure tag
Comment #181
Gábor HojtsyI'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!
Comment #182
larowlanDocs added here http://drupal.org/node/1934442
Comment #183.0
(not verified) CreditAttribution: commentedUpdated issue summary.