Support from Acquia helps fund testing for Drupal Acquia logo

Comments

batigolix’s picture

Surprisingly the Mother of all Documentation got in core without documentation ;)

1st attempt. I included a little snear from the Help text team to the Tour team (fierce competition over who has the most beautiful documentation in Drupal)

;)

Note to who tests this patch:
No entry for Tour appears at the /admin/help page of my local (slightly buggy) D8 install

batigolix’s picture

Status: Active » Needs review

setting status

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Ha ha

jhodgdon’s picture

Component: documentation » tour.module
Status: Reviewed & tested by the community » Needs work

I think the text needs a bit of help with formatting and grammar:

a) Drupal coding standards require that text with ' in it be enclosed in "" not ''

b) The link text for the help on how to write tours is very long... Can't the link be combined with the standard "for more information see the online documentation" sentence instead, something like "For more information on the Tour module see ... and for information on how to write tours, see ..."?

c) Let's get rid of this sentence: Tours are the ideal documentation for users who like to click a lot. It tells me nothing useful.

d) Maybe there should be a reference to the Help module or a description of how they are different?

e) I'm moving this temporarily to the Tour module component so the maintainers can comment on accuracy, since I don't think they've commented here. ... So I'm not familiar with this module at all, but it's called "tours" not "tooltips". Does it really do tours somehow? As the text is written, it just sounds like it gives me tooltips for individual UI interface elements, and that doesn't sound like a "tour". What does this module really do?

larowlan’s picture

Assigned: Unassigned » larowlan

So after irc discussion we decided the uses for tour should be documented as
1) Viewing a tour
2) Creating a tour (links to api docs on d.o)
with an about for what it is.
I'll take a run at this.

larowlan’s picture

Component: tour.module » documentation
Assigned: larowlan » Unassigned
Status: Needs work » Needs review
FileSize
53.24 KB
1.51 KB
2.15 KB

Hi @jhodgdon
How does this look?
Lee
Screen Shot 2013-07-09 at 11.14.37 AM.png

jhodgdon’s picture

Component: documentation » tour.module
Status: Needs review » Needs work

Let's leave this in tour.module until the maintainers comment on accuracy.

I have a few questions/comments:

a) If the Tour button is only visible on the Toolbar, shouldn't we mention that you need the Toolbar module enabled? (or maybe it is required by Tour, in which case we don't need to mention it I guess).

b) The Uses section is not formatted in our standard way for Uses: http://drupal.org/node/632280

I would suggest two individual Uses headings:
- Viewing a tour
- Creating a tour
The Viewing section should contain this text from About (which shouldn't be in About anyway, since it tells how to use the module): "When a tour is available on a page .... that page's interface."

The Creating section should have a link to the online documentation on how to create tours, which I think was in a previous version of this patch.

c) The About section should have a link to the online docs. See the help standards link above for format.

larowlan’s picture

Hi @jhodgdon
Here it is updated.
Good catch on the dependency on toolbar, which I've added and discussed with the maintainer (@nick_schuch)
FWIW I was a significant contributor to the module, you can read the back-story in my blog post here http://previousnext.com.au/blog/tour-module-part-1-journey-adding-drupal...
I would have put my hand up as a maintainer too but I'm already down for forum and custom_block and conscious of spreading myself too thin.

Here's how it looks
Screen Shot 2013-07-10 at 9.08.22 AM.png

Lee

nick_schuch’s picture

Hi @jhodgdon

This reads great!

Thankyou!

jhodgdon’s picture

Status: Needs review » Needs work

I think it looks good too!

So let's just clean up a couple of style/grammar/punctuation things:

a) "A tour may also recommend additional related tours on the site to the user; which can be taken in sequence to provide a walkthrough of the site in a structured manner."
- ; should be , here
- I would leave out the word "additional", which does not provide any additional added related information. :)

b) "...to the user, guide the user through a workflow or explain key Drupal concepts."
- Our Drupal convention is to use serial commas, so there needs to be a comma before "or"

c) The tour development page - we like to have pages referenced from help have better URLs than node/something. So I added an alias to the page: https://drupal.org/developing/api/tour

d) "the online documentation for the writing tours"
- Remove second "the"

e) In About, let's say "...overview of user interfaces..." rather than "overview of various interfaces". It already says "throughout the site" so I don't think we need the word "various".

f) Throughout, I think we should use the term "user interface" rather than just "interface"?

nick_schuch’s picture

Status: Needs work » Needs review
FileSize
2.18 KB
2.75 KB

Here are the changes as outlined in #10.

jhodgdon’s picture

Status: Needs review » Needs work

(b) from #10 still needs attention -- still missing comma before "or" in:
"Tours can be used to highlight critical components of the user interface to the user, guide the user through a workflow or explain key Drupal concepts."

Other than that, looks great! Someone should also manually test to make sure it displays correctly and the links work.

batigolix’s picture

Status: Needs work » Needs review
FileSize
1.9 KB

This display correctly & the links work.

The attached patch adds the missing comma.

jhodgdon’s picture

Component: tour.module » documentation
Status: Needs review » Reviewed & tested by the community

Great! The maintainers of Tour are on board, the links and formatting have been independently tested, and I think all the style issues have been addressed. Let's get this one in.

batigolix’s picture

first rtbc patch for the D8 hook_help fixup!

drum roll please

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

Oh dang.

I just found an error in this patch. For some reason whoever wrote the t() part for About decided to split it into multiple lines, which is OK, but if you do that with array() you need to have a comma at the end of each line, which wasn't done.

+      $output .= '<p>' . t("The Tour module provides users with a context-sensitive overview of user interfaces throughout the site. For more information, see <a href='@tour'>the online documentation for the Tour module</a>.", array(
+        '@tour' => 'https://drupal.org/documentation/modules/tour'
+      )) . '</p>';

https://drupal.org/coding-standards#array

We are not splitting up any other hook_help t() sections this way and I'd prefer just to see it on one line like the others anyway.

batigolix’s picture

Patch:

- addresses #16
- changes @token to !token

batigolix’s picture

Status: Needs work » Needs review

changing status

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

I think this is ready to go! Thanks!

jhodgdon’s picture

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

Oh wait. Actually, someone needs to manually test the links and make sure they all still work.

-enzo-’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
85.9 KB

I tested the patch successfully to get the help you must visit the page http://example.com/admin/help/tour, remember clear cache, because if you apply the patch the page will not be available, due the tour module is already enabled.

You will get a page similar to this.

tested_tour_help.png

clemens.tolboom’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Tour
FileSize
3.08 KB
2.87 KB

Hmm not tagged with Tour so I missed this completely.

I just added a patch to #1920470: Find out how help and tour can work together which was cross post by @jhodgdon and @clemens.tolboom

As per request of @jhodgdon in #1920470-20: Find out how help and tour can work together I add my patch too #17

I changed contrib modules to Tour UI and of course added the Tour list.

My notes are still valid:

  1. Trouble is not all path are complete. Probably most are not.
  2. The patch does not use any caching so is slowing down /admin/help and /admin/help/tour so needs work if this is a step forward.
petrpo’s picture

Issue tags: -Tour
+++ b/core/modules/tour/tour.module
@@ -20,8 +20,36 @@ function tour_help($path, $arg) {
-      $output .= '<dd>' . t("Tours can be used to highlight critical components of the user interface to the user, guide the user through a workflow, or explain key Drupal concepts. Tours can be written as YAML-documents with a text editor, or using contributed modules. For more information, see <a href='!doc_url'>the online documentation for writing tours</a>.", array('!doc_url' => 'https://drupal.org/developing/api/tour')) . '</dd>';

interdiff-2035145-17-22.txt : It looks done Ok.

clemens.tolboom’s picture

Issue tags: +Tour

I added the tour tag as that makes it appear on https://drupal.org/project/issues/search/drupal?status[]=Open&version[]=... which is vital.

As the documentation is now OK (I hope) shall we place component back to tour.module? My code additions need special care.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks for the patches, and sorry for the delay in reviewing -- the help sprint was so successful, I have been having trouble catching up!

The code within hook_help() needs an in-line comment at the top explaining what it is doing. Also, I don't think the title of the list of links should be "Available tours." with a . at the end -- it is not a sentence. And I'd like to see what this looks like.

We're doing something similar in the Field module help... that code has been there since D7... is the formatting compatible?

clemens.tolboom’s picture

Status: Needs work » Needs review
FileSize
1.32 KB

Attached patch fixes small issues from #25

@jhodgdon: Comparing with the field.module I do not see what you mean?

  1. +++ b/core/modules/tour/tour.module
    @@ -7,6 +7,54 @@
    +      $names = drupal_container()->get('config.storage')->listAll('tour.tour.');
    

    Add comment explaining

  2. +++ b/core/modules/tour/tour.module
    @@ -7,6 +7,54 @@
    +      $output .= theme('item_list', array('items' => $tours, 'title' => 'Available tours.'));
    

    Remove period.

clemens.tolboom’s picture

(argh) Consider #26 as the interdiff :(

clemens.tolboom’s picture

  1. +++ b/core/modules/tour/tour.module
    @@ -7,6 +7,55 @@
    +      $names = drupal_container()->get('config.storage')->listAll('tour.tour.');
    

    Can we do better then getting the list of names?

  2. +++ b/core/modules/tour/tour.module
    @@ -7,6 +7,55 @@
    +        // Only handle elements with a schema. The schema system falls back on the
    +        // Property class for unknown types. See http://drupal.org/node/1905230
    +        $definition = config_typed()->getDefinition($name);
    +        if (is_array($definition) && $definition['class'] == '\Drupal\Core\Config\Schema\Property') {
    +          continue;
    +        }
    

    Can't we assume tour.tour. gives correct config items and thus no need to check for this?

    Note: This is taken from Tour UI module

  3. +++ b/core/modules/tour/tour.module
    @@ -7,6 +7,55 @@
    +        $tips = count($tour['tips']);
    

    getValue() is not really OO and is relying on the internals. But we need to get to the label, path and tips.

  4. +++ b/core/modules/tour/tour.module
    @@ -7,6 +7,55 @@
    +        $tours[] = $label . ' (' . $tips . '): ' . $link;
    

    Is it useful to list the number of tips available?

jhodgdon’s picture

Regarding field.module, its help makes a list of field modules, after the text. All I meant was that we should try to use similar formatting here for the list of tours.

jhodgdon’s picture

Status: Needs review » Needs work

Seems like due to #28, this needs some work.

batigolix’s picture

  1. What information do we display in the list of available tours if no tours are available.
  2. You add a condition that checks for an argument * in the path. If it is found just the path is displayed (see attachment)
    +          if (strpos($path, "*") === FALSE) {
    +            $links[] = l($path, $path, array('query' => array('tour' => 1)));
    +          }
    +          else {
    +            $links[] = check_plain($path);
    +          }
    

    Is that really useful? It results in this:
    tour help with tours list

  3. The formatting in the field help text is also a unordered list but it has not got its own header:
    The Field module provides the infrastructure [SNIP] Currently enabled field and input widget modules:
     <div class="item-list">
      <ul>
       <li class="odd first"><a href="/admin/help/email">E-mail</a></li>
       <li class="even"><a href="/admin/help/entity_reference">Entity Reference</a></li>
       <li class="odd"><a href="/admin/help/file">File</a></li>
       [SNIP]
      </ul>
     </div>
    
clemens.tolboom’s picture

@batigolix Is it an idea to update the issue summary with all open issues from us?

#31 #1 We could just tell that:

"No tour are available yet. Maybe enable some modules like Views UI."

#31 #2 Yes it is useful to should the path. It indicates a 'for all path'. But path are now becoming routes so not sure.

#31 #3 I don't get the point :/

batigolix’s picture

#31 #3 see in #25 the question about the formatting of the list in the field help page

batigolix’s picture

Can the code that generates the available tours become a seperate function outside of the hook_help, like e.g. tour_show_available_tours() ?

Then we can finish the hook_help text now and improve it the available tours list later when we have a better idea of which tours will be in D8 (at the moment there is still only that 1 tour for views_ui)

batigolix’s picture

Component: documentation » tour.module
Status: Needs work » Needs review
Issue tags: +Novice
FileSize
1.9 KB

Kind of completely rewrote the earlier proposals, removing all brochure speak ("ours can be used to highlight critical components", "The Tour module provides users with a context-sensitive overview of user interfaces").

Also removed the code that generates the available tours list. It is currently broken and I'm not sure if it is very helpful at the moment. Let's add it later when (a) it works, (b) we have more than 1 tours in core.

Links & reference to the UI have been verified.

Also changing component to tour.module for maintainers feedback.

batigolix’s picture

FileSize
4.02 KB

interdiff for #35

jhodgdon’s picture

Status: Needs review » Needs work

I like the rewrite.

Two minor issues:
a)

If a tour is available, a <em>Tour</em> button will be visible in the toolbar.

Do you think this would be clearer if it said "If a tour is available on a page, ..."? (At least, my understanding is that you have to be on a particular page with a tour defined in order to see the Tour button?)

b)

To see a tour users must have the  permission ...

There is an extra space before the word "permission".

batigolix’s picture

Status: Needs work » Needs review
FileSize
1.89 KB
1.86 KB

Patch fixes points from #37

Status: Needs review » Needs work

The last submitted patch, 38: hook-help-tour-2035145-38.patch, failed testing.

drupalninja99’s picture

Status: Needs work » Needs review
FileSize
1.9 KB

I couldn't get #38 to apply so I manually re-applied the changes from #38. It looks like the use statement at the top of #38 is wrong. At any rate, here is the updated patch.

jhodgdon’s picture

Thanks! Yes it looks like the Tour module has a different line at the top so the context part of the patch didn't work any more.

So I think this patch is good. It just needs a quick round of manual testing:
- Verify that all the links work
- Verify that all mentions of pages/text/permissions within the UI match what is seen in the UI
- Verify that the formatting is OK.

larowlan’s picture

With regards to the list of tours, we resolved in our last scrum to make this a block, possibly built with views, as placing the list on other pages as well (not just help pages) is a definite possibility.
I will test as asked on Monday

Nitesh Sethia’s picture

Assigned: Unassigned » Nitesh Sethia
Nitesh Sethia’s picture

Assigned: Nitesh Sethia » Unassigned
Status: Needs review » Reviewed & tested by the community
jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Thanks again everyone -- committed to 8.x.

Status: Fixed » Closed (fixed)

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