Background:
This issue is part of the task to update the hook_help texts of the Drupal 8 modules:
#1908570: [meta] Update or create hook_help() texts for D8 core modules

Tasks:
- review / write the hook_help text according to help guidelines

Files: 
CommentFileSizeAuthor
#28 toolbar-updated-help-documentation_2091379-28.patch2.76 KBgreggmarshall
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,923 pass(es).
[ View ]
#28 interdiff-2091379-21-28.txt1.85 KBgreggmarshall
#21 interdiff_2091379-19-21.txt1.63 KBer.pushpinderrana
#21 toolbar-updated-help-documentation_2091379-21.patch2.63 KBer.pushpinderrana
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,862 pass(es).
[ View ]
#25 interdiff-2091379-21-25.txt1.85 KBgreggmarshall
#25 toolbar-updated-help-documentation_2091379-25.patch1.85 KBgreggmarshall
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch toolbar-updated-help-documentation_2091379-25_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

petrpo’s picture

Trying to correct it according to standards

petrpo’s picture

Status:Active» Needs review
StatusFileSize
new2.12 KB
PASSED: [[SimpleTest]]: [MySQL] 58,654 pass(es).
[ View ]
jhodgdon’s picture

Status:Needs review» Needs work

Thanks -- this looks good! A couple of things still need to be fixed:

a) Small standards fix: When referring to the module, say "the Toolbar module". "the" is missing in the last sentence of About.

b) I also noticed a small grammatical problem:

The drawer can be hidden/shown by clicking on its corresponding tab.

The previous sentences refer to several drawers, not one drawer. So this should probably say "Each drawer can be..."?

petrpo’s picture

StatusFileSize
new2.1 KB
PASSED: [[SimpleTest]]: [MySQL] 58,761 pass(es).
[ View ]

a) & b) suggestions are logical. I corrected them in this patch.

petrpo’s picture

Status:Needs work» Needs review

simpletest.me went ok & qa as well. Ready for comments and reviews.

jhodgdon’s picture

Status:Needs review» Needs work

Thanks! I think the help reads well now. However, the patch in #4 does not have the URL standards fixes from the patch in #2. Whoops! Just need to combine them. :)

batigolix’s picture

The toolbar has changed quite a bit:

  • It now consists of two rows of links, one being top level menu entries and the second being the top level adminisitration items as we know them.
  • The second bar can be toggled to be displayed vertically, where each item can be collapsed into lower level admin item (emulating contrib modules like admin_menu a bit).
  • On narrow screens the second bar is displayed vertically by default.

This is not currently mentioned in the help text.

jhodgdon’s picture

We just had a change to hook_help, on this issue: #2183113: Update hook_help signature to use route_name instead of path.

Here is the change record: https://drupal.org/node/2250345

This patch will need a reroll for this change.

amitgoyal’s picture

Status:Needs work» Needs review
StatusFileSize
new2.44 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,679 pass(es).
[ View ]

@jhodgdon - Based on the suggestion in #7 and the way new toolbar works, I have made some changes in the help text.

Please see if it looks good to you as well.

jhodgdon’s picture

Status:Needs review» Needs work

Thanks for the new patch!

I think we should keep the description in "About" fairly general, not getting into details about the two rows of links etc. The first sentence, in particular, should just give a short description of what the module does. The details should go in Uses sections, as they were before.

Also, we should not have sections explaining how to use the Shortcut module. We should *mention* the Shortcut module and link to its help page for details, and explain where/how the shortcuts can be displayed within Toolbar, but we shouldn't give details about how to edit shortcuts or add a shortcut link in the Toolbar help.

amitgoyal’s picture

Status:Needs work» Needs review
StatusFileSize
new2.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,624 pass(es).
[ View ]
new2.89 KB

@jhodgdon - Thanks for your feedback.

I have updated the About / Uses sections based on your feedback. I have also corrected URL format and online help link.

Please review.

jhodgdon’s picture

Status:Needs review» Needs work

Thanks! I still think this needs some work... There is still a big section that is specific to Shortcuts, but it shouldn't be (actually the text there applies to either the main menu or the shortcuts, whatever is displayed in the "drawer" that is currently visible, doesn't it?).

So... I just tried this module out (hadn't played with it in a while). Here's the behavior I think we should be documenting in the help:

a) Toolbar displays "tab" and "tray" content from other modules. Core modules such as Shortcut, Edit, and Menu provide tabs and trays.
b) Tabs are usually buttons, and are displayed in a bar across the top of the screen. Some tabs are simple buttons that execute an action (such as starting Edit mode); other tabs toggle which tray is open.
c) Trays are usually lists of links, which can be hierarchical like a menu. If a tray has been toggled open, it is displayed either vertically or horizontally below the tab bar, depending on the browser width. In wide browser widths, the user has the ability to toggle from vertical to horizontal, using a link at the bottom or right of the tray; hierarchical menus only have open/close behavior in vertical mode.

I'm not suggesting this exact wording, but I think this is what needs to be documented for this module.

amitgoyal’s picture

Status:Needs work» Needs review
StatusFileSize
new3.05 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,806 pass(es).
[ View ]
new3.56 KB

@jhodgdon - Suggested text looks good. I have placed the Tabs/Trays descriptions under Terminology section like Breakpoint module.

I have also removed Shortcuts section, as suggested. Please see the revised help for Toolbar module.

jhodgdon’s picture

Status:Needs review» Needs work

Better, thanks!

So... In the first line of About:

"The Toolbar module displays a bar containing top-level administrative components across the top of the screen."

We need a better summary...

Maybe:

The Toolbar module provides a toolbar for site administrators, which displays tabs and trays provided by the Toolbar module itself and other modules.

(since you've defined tabs and trays below anyway). I think the text in About in the Terminology section is very good.

So, then the Uses section... I wonder if we even need it, since the About section, with its terminology, describes the uses? Let's just get rid of it. I do not think it is adding any information.

Thanks!

amitgoyal’s picture

Status:Needs work» Needs review
StatusFileSize
new2.54 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,798 pass(es).
[ View ]
new2.55 KB

Sounds better! I have made the required changes.

Please review.

jhodgdon’s picture

Status:Needs review» Reviewed & tested by the community

Looks good to me! Thanks!

paboden’s picture

StatusFileSize
new2.41 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,751 pass(es).
[ View ]

Checked this one out to give it additional community review, and had some grammar and text cleanup thoughts. I worked off of #15 patch.

I made edits to the tabs and trays definitions, and felt the following text might be a little cleaner:

Tabs are buttons, displayed in a bar across the top of the screen. Some tabs execute an action (such as starting Edit mode), while other tabs toggle which tray is open.
Trays are lists of links, displayed either vertically or horizontally below the tab bar, depending on the browser width. In wide browser widths, the user has the ability to toggle from vertical to horizontal, using a link at the bottom or right of the tray. Tray links only have open/close behavior in vertical mode.

Feel free to incorporate or not. I added a patch just in case. This was just based on how the content hit me the first time I read through it.
Cheers

jhodgdon’s picture

Status:Reviewed & tested by the community» Needs work

Hi paboden! Welcome to this issue and contributing!

A couple of things:

a) If you add a new patch to an issue, you need to set the status back to "Needs review" because it is a new patch and needs reviewing, so the "reviewed and tested" status is not correct.

b) When adding a patch to an issue that already had a patch, an interdiff file is also requested: https://drupal.org/documentation/git/interdiff

So... Anyway, I reviewed your new patch.

I like the first change you made, to the part about tabs.

I am not as positive about the second change you made. I think the information about hierarchical lists is important, and it's also important to note that the hierarchical ones do not work as well in horizontal mode (which is counter to what people might expect since many horizontal menus in general on the web have drop-down behavior). And changing it to say "Tray links only have open/close behavior in vertical mode." is confusing. The previous text was, I think, better?

I am also not sure about removing the "usually" in the trays (in tabs I think it is OK). If you look at the developer documentation for the Toolbar module, you will see that modules can return whatever content they want for a tray. It doesn't have to be a list of links as far as I can tell. See:
https://api.drupal.org/api/drupal/core!modules!toolbar!toolbar.api.php/f...

Also... I am seeing now that in the hook_toolbar documentation, the "tabs" we have documented here are also called "toggles". Maybe that would be a better term to use in this help as well? It does seem clearer than "tabs"

rootwork’s picture

Status:Needs work» Needs review
StatusFileSize
new1.43 KB
new2.52 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,481 pass(es).
[ View ]

Just to move this forward, I'm attaching a new patch which incorporates paboden's text for "Tabs" and amitgoyal's earlier text for "Trays," per jhodgdon's review.

Jennifer, I'm not sure I understand what you're saying in the last part, about tabs being called toggles. The hook documentation says:

The tray may be associated with a toggle in the administration bar. The toggle shows or hides the tray and is optimized for small and large screens. To create this association, hook_toolbar() returns one or more render elements of type 'toolbar_item', containing the toggle and tray elements in its 'tab' and 'tray' properties.

To me that says the element to show/hide trays is a "toggle" but the elements within a tray is a "tab." Directly under that text it says that the first property available is "'tab': A renderable array."

But maybe I'm not understanding what you're meaning?

I do think it would make more sense to call this something more generic -- "button" would make more sense to me than "toggle," especially since the first line of the help text says "Tabs are buttons." But in that case I think we should change the name of the array from "tab."

Anyway, here's a revised patch and interdiff.

jhodgdon’s picture

Status:Needs review» Needs work

I think this help is looking very good!

The only thing that I think is missing is that it doesn't explicitly mention that you can only have one tray open at a time, which I am pretty sure is true: if you open a try, any other open trays will close, right?

And maybe we should explicitly say that if you display a hierarchical tray horizontally, you only see the top level of links? It is kind of implied but maybe not quite obvious from the text that is there... There is no harm in making it obvious. :) So maybe take this phrase:
; hierarchical menus only have open/close behavior in vertical mode.

and make it into a separate sentence...

Hierarchical menus only have open/close behavior in vertical mode; if you display a tray containing a hierarchical menu horizontally, only the top-level links will be available.

er.pushpinderrana’s picture

Status:Needs work» Needs review
StatusFileSize
new1.63 KB
new2.63 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,862 pass(es).
[ View ]

Please review updated patch with #20 changes.

jhodgdon’s picture

Status:Needs review» Needs work

That looks good... it doesn't include this change though from #20:

... it doesn't explicitly mention that you can only have one tray open at a time, which I am pretty sure is true: if you open a try, any other open trays will close, right?

batigolix’s picture

Issue tags:+documentation, +sprint
greggmarshall’s picture

Assigned:Unassigned» greggmarshall
greggmarshall’s picture

Status:Needs work» Needs review
StatusFileSize
new1.85 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch toolbar-updated-help-documentation_2091379-25_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.85 KB

I had to re-roll the patch because the Shortcut URL had been changed from a relative path to using drupal::URL, but that will be deleted by this patch anyway...

I added

Only one tray may be open at a time. If you click another tab, that tray will replace the tray being displayed.

On my plain Drupal 8 installation, I could verify that only one tray can be open at a time. Clicking on each tab would open the corresponding try, replacing the open tray. In a base Drupal 8 installation, there are no tabs without trays, so I couldn't test what happens if you click on a tab that is an action.

batigolix’s picture

Assigned:greggmarshall» Unassigned

The last submitted patch, 25: toolbar-updated-help-documentation_2091379-25.patch, failed testing.

greggmarshall’s picture

StatusFileSize
new1.85 KB
new2.76 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,923 pass(es).
[ View ]

Fixed patch to base off 8.0.x and not result of patch #21.

Status:Needs review» Needs work

The last submitted patch, 28: toolbar-updated-help-documentation_2091379-28.patch, failed testing.

Status:Needs work» Needs review
jhodgdon’s picture

Status:Needs review» Reviewed & tested by the community

This patch looks great to me, thanks!

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed 24bc995 and pushed to 8.0.x. Thanks!

  • alexpott committed 24bc995 on 8.0.x
    Issue #2091379 by amitgoyal, greggmarshall, petrpo, rootwork, er....

Status:Fixed» Closed (fixed)

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