Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Background:
This issue is part of the task to update/create the hook_help texts of the modules for Drupal 8:
#1908570: [meta] Update or create hook_help() texts for D8 core modules
Tasks:
- write the hook_help function
- review d.o. docs at http://drupal.org/documentation/modules/tour
Comment | File | Size | Author |
---|---|---|---|
#40 | hook-help-tour-2035145-40.patch | 1.9 KB | drupalninja99 |
#38 | interdiff.txt | 1.86 KB | batigolix |
#38 | hook-help-tour-2035145-38.patch | 1.89 KB | batigolix |
#36 | interdiff.txt | 4.02 KB | batigolix |
#35 | hook-help-tour-2035145-35.patch | 1.9 KB | batigolix |
Comments
Comment #1
batigolixSurprisingly 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
Comment #2
batigolixsetting status
Comment #3
larowlanHa ha
Comment #4
jhodgdonI 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?
Comment #5
larowlanSo 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.
Comment #6
larowlanHi @jhodgdon
How does this look?
Lee
Comment #7
jhodgdonLet'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.
Comment #8
larowlanHi @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
Lee
Comment #9
nick_schuch CreditAttribution: nick_schuch commentedHi @jhodgdon
This reads great!
Thankyou!
Comment #10
jhodgdonI 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"?
Comment #11
nick_schuch CreditAttribution: nick_schuch commentedHere are the changes as outlined in #10.
Comment #12
jhodgdon(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.
Comment #13
batigolixThis display correctly & the links work.
The attached patch adds the missing comma.
Comment #14
jhodgdonGreat! 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.
Comment #15
batigolixfirst rtbc patch for the D8 hook_help fixup!
drum roll please
Comment #16
jhodgdonOh 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.
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.
Comment #17
batigolixPatch:
- addresses #16
- changes @token to !token
Comment #18
batigolixchanging status
Comment #19
jhodgdonI think this is ready to go! Thanks!
Comment #20
jhodgdonOh wait. Actually, someone needs to manually test the links and make sure they all still work.
Comment #21
-enzo- CreditAttribution: -enzo- commentedI 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.
Comment #22
clemens.tolboomHmm 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:
Comment #23
petrpo CreditAttribution: petrpo commentedinterdiff-2035145-17-22.txt : It looks done Ok.
Comment #24
clemens.tolboomI 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.
Comment #25
jhodgdonThanks 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?
Comment #26
clemens.tolboomAttached patch fixes small issues from #25
@jhodgdon: Comparing with the field.module I do not see what you mean?
Add comment explaining
Remove period.
Comment #27
clemens.tolboom(argh) Consider #26 as the interdiff :(
Comment #28
clemens.tolboomCan we do better then getting the list of names?
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
getValue() is not really OO and is relying on the internals. But we need to get to the label, path and tips.
Is it useful to list the number of tips available?
Comment #29
jhodgdonRegarding 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.
Comment #30
jhodgdonSeems like due to #28, this needs some work.
Comment #31
batigolixIs that really useful? It results in this:
Comment #32
clemens.tolboom@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 :/
Comment #33
batigolix#31 #3 see in #25 the question about the formatting of the list in the field help page
Comment #34
batigolixCan 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)
Comment #35
batigolixKind 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.
Comment #36
batigolixinterdiff for #35
Comment #37
jhodgdonI like the rewrite.
Two minor issues:
a)
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)
There is an extra space before the word "permission".
Comment #38
batigolixPatch fixes points from #37
Comment #40
drupalninja99 CreditAttribution: drupalninja99 commentedI 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.
Comment #41
jhodgdonThanks! 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.
Comment #42
larowlanWith 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
Comment #43
Nitesh Sethia CreditAttribution: Nitesh Sethia commentedComment #44
Nitesh Sethia CreditAttribution: Nitesh Sethia commentedComment #45
jhodgdonThanks again everyone -- committed to 8.x.