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
#54 contextual-help-text-2091311-54.patch3.16 KBer.pushpinderrana
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,805 pass(es).
[ View ]
#51 contextual-help-text-2091311-46.patch3.15 KBjhodgdon
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,778 pass(es).
[ View ]

Comments

wzoom’s picture

Assigned:Unassigned» wzoom

I am working on the docs.

wzoom’s picture

Assigned:wzoom» Unassigned
Status:Active» Needs review
StatusFileSize
new2.97 KB
PASSED: [[SimpleTest]]: [MySQL] 58,460 pass(es).
[ View ]

I am proposing the first draft in the patch. I think it needs some proofreading/corrections. Feel free to change it as needed.

batigolix’s picture

Status:Needs review» Needs work

The change you made to the link is okay.

The changes in the last paragraph are confusing compared to the original. I think the original sentence "... adds JavaScript code to the page to hide the links initially, and display them when your mouse hovers over the block." was/is okay and does not require changes. I would revert it.

The link to d.o should become https://drupal.org/documentation/modules/contextual

batigolix’s picture

Oh, I see now that this module provides a pencil icon in the toolbar that "reveals" the contextual link pencil icons elsewhere on the page. Your text makes a bit more sense to me now. The part about JavaScript & mouse hovering is still valid though and shouldnt be removed.

wzoom’s picture

Status:Needs work» Needs review
StatusFileSize
new2.62 KB
new2.23 KB
PASSED: [[SimpleTest]]: [MySQL] 59,467 pass(es).
[ View ]

Ok, thank you.
I reverted the "JavaScript sentence" to the original and changed the link to https.

jhodgdon’s picture

Status:Needs review» Needs work

Thanks for the patches!

A couple of things still need to be fixed:

a) We need to make sure that what is in this help matches what a user would see in the Drupal UI.

For instance, in About there is a permission mentioned 'access contextual links'. This is the machine name of the permission; a user would see "Use contextual links", so that is what should be in the help text.

b) The link to the online docs -- there should be more text inside the link. Our template/standards are at:
http://drupal.org/node/632280

c) This sentence at the end is not grammatical: "Initially the links are hidden, and displayed only when your mouse hovers over the configurable area."

d) I think the text about the pencil icon is confusing and maybe innacurate. What I'm seeing is that each block on the page (including the main block) has a pencil that shows up when I hover over the block. Clicking the pencil exposes a drop-down menu, and also makes the pencil stay visible. Clicking the pencil again or clicking another block's pencil makes the other pencil hidden (only one pencil on the page can apparently be visible at a time). That doesn't seem to mesh with what the text says, and truthfully, I have no idea why keeping a pencil icon in "shown" mode is even useful, so I don't really think there is a strong reason to document it?

batigolix’s picture

Status:Needs work» Needs review
StatusFileSize
new2.28 KB
PASSED: [[SimpleTest]]: [MySQL] 59,929 pass(es).
[ View ]

Patch addresses the points made in #6

I tried to rewrite the section about the edit mode

jhodgdon’s picture

Status:Needs review» Needs work

Interdiff, please! :)
https://drupal.org/documentation/git/interdiff
(next time, anyway).

So, I finally understand what everyone was trying to say in the toggle section, and this is functionality I had never noticed. Good job on the wording!

I think, however, that rather than saying "the administrative toolbar", we should probably say "the toolbar provided by the Toolbar module" (with a link to the toolbar help). I also don't think the statement that it is only available in some things is accurate -- the Toolbar seems to display the same no matter what theme is being used. However, I found it was not available on all *pages* -- for instance, on the Appearance (themes) page, there is nothing to edit so there are no available pencil icons.

So, maybe it would be a good idea to mention that the pencil icon in the toolbar only appears if there are editable elements on the page?

There is one other problem with this help. I don't think we should assume that everyone can see the pencil icons. In the toolbar, someone using a screen reader would hear "Edit", and on the other pencil icons in the page, they would hear "Open ... configuration options' -- they would not know what a "pencil icon" is, because it is not called a "pencil icon" in the accessible text. Also, technically, clicking the pencil in the toolbar doesn't display the contextual links. It just makes all the pencil icons on the page visible, and then clicking an individual pencil icon is what actually shows you the contextual links.

batigolix’s picture

Do we have examples in other hook_help texts on how we dealt with icons in the UI?

jhodgdon’s picture

I don't know of any other hook_help() texts that have mentioned icons.

We could certainly include an image tag in the help if we wanted one there, but in this case we really do need to consider accessibility.

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.

mparker17’s picture

Assigned:Unassigned» mparker17

I'll help!

mparker17’s picture

Commit no-longer applies. Re-rolling.

mparker17’s picture

StatusFileSize
new2.59 KB

Straight re-roll, therefore no interdiff.

Method argument change mentioned in #11 was fixed automatically.

mparker17’s picture

Assigned:mparker17» Unassigned
Status:Needs work» Needs review
StatusFileSize
new2.63 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,808 pass(es).
[ View ]
new1.81 KB

I tried to address all the feedback in #8 in this patch.

jhodgdon’s picture

Status:Needs review» Needs work

Thanks -- I really like the wording here!

Question: Is the Pencil icon displayed whenever there are any contextual links, or just when there is "editable" information on the page? Because this concept of "editable" is not really explained, if it is just when there is "editable" information. If the pencil means "turn on all contextual links", then we should say it does that. If it is for "editable", then we should explain exactly what subset of contextual links are considered "editable". Either way, this is "needs work"... other than that, I think this is looking good though!

mparker17’s picture

Most of the code to enable this "Pencil icon" in the toolbar is in the Contextual module.

Looking at the code in core/modules/contextual/js/contextual.toolbar.js::initContextualToolbar() and core/modules/contextual/js/contextual.js::Drupal.behaviors.contextual(), as far as I can tell, the icon in the toolbar appears if and only if there are regions with contextual links on the page (i.e.: if there are HTML elements that contain a data-contextual-id attribute).

However, as my JavaScript skills are rustier than I'd like them to be, I'd prefer if someone more familiar with the Contextual module was able to confirm that.

jhodgdon’s picture

OK good, thanks. That was my impression as well. Going on that idea... let's not talk about "editable" information in this help then.

mparker17’s picture

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

Feedback welcome!

jhodgdon’s picture

Status:Needs review» Needs work

I tried out Contextual Links today while testing the help for Quick Edit on #2091401: Add hook_help for Quick Edit module, and I think you're right about when the button is visible in the toolbar -- visible when there is at least one region with contextual links being displayed on the page.

So... on that other issue, I suggested text like this... and then decided the explanation belonged in Contextual Links help instead (without the reference to Quick Edit or the content area -- using the Blocks example in this current help text would be better):

Quick edit mode is activated via the (link to contextual links help)contextual links(endlink) button (which looks like a pencil in most themes, and is normally displayed in the upper right corner of the content area). There are two ways to make the contextual links button visible:
- Hovering over the area where the content is displayed will temporarily make the button visible.
- If you have the (link to toolbar help)Toolbar module(endlink) enabled, clicking the contextual links button in the toolbar (which looks like a pencil) will make all contextual links on the page visible. Clicking this button again will toggle them to invisible.
In either case, click the contextual links button for the content you want to edit, and choose Quick edit to activate quick edit mode.

Can we incorporate something like this into the help (with the menu block example substituted)? I think this is clearer than separating the Uses into "displaying" and "toggling edit mode" (and I still think "edit mode" is not a good term to use).

Really this module help just needs to
- explain what contextual links are (which I think it does a good job at -- but probably the explanation should be in About and not Uses?
- tell how to recognize contextual links (pencil icon in upper right corner of the region, see suggested text in this comment)
- explain the two ways that contextual links for a given region on the page can be activated (hover or the toolbar button, see suggested text in this comment)

And I like the "region" terminology in the current patch... let's make sure it's used consistently... or ... maybe it would be better as "area" so as not to confuse with the regions in your theme, which are not the same thing?

mparker17’s picture

StatusFileSize
new3.16 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,753 pass(es).
[ View ]
new3.33 KB

Contextual links are not available for Regions (as defined by the Theme System), but they are available for things that go inside Regions (esp. Blocks), so I think it is wise to refer to them as "areas".

I've taken a stab at implementing all the suggested changes in #20, see attached patch. Feedback welcome!

The only thing I'm not sure about is the refactored sentence:

Contextual links give you quick access to tasks associated with certain areas of pages on your site.

... emphasis on the part that sounds weird to me (I know I wrote it; but I couldn't think of any better way of saying it).

mparker17’s picture

Status:Needs work» Needs review

Whoops, Issue Status.

jhodgdon’s picture

Status:Needs review» Needs work

Thanks! This is better... but I think it could be improved:

a)
I think your refactored sentence is clear enough, especially since right afterwards, the next sentence starts with "For example" ...

But... let's read that next sentence again:

For example, the Blocks and Menu UI modules supply links to configure the block and edit the menu.

I think one of your previous patches had an explanation that is missing here, which said something about assuming you were displaying the menu in a block? Without that, it doesn't make any sense.

b) Starting over at the beginning:

Contextual links give you quick access to tasks ... Only users with the <em>Use contextual links</em> permission will be able to view Contextual links.

In most other of our module help pages, we would be more likely to not say "you" in the first sentence, but instead "Contextual links give users with xyz permission ..." (and eliminate the 2nd sentence entirely).

c) We also normally start out module overviews with "The Foo Bar module ", so that it's clear we're telling what this module does.

d) In the Uses section... All of a sudden we are talking about content editing, which is by far not the only task that contextual links are for (this text came from my Quick Edit module help I think, but it needs to be adapted). I think we should take out the references to content and editing, and make references instead to the page area that the contextual links are related to, and the tasks you can select?

e) We're still missing a description of how to recognize the contextual link buttons. In my proposed text for Quick Edit, I said:

... contextual links button (which looks like a pencil in most themes, and is normally displayed in the upper right corner of the area). ...

Can we work something like that into the help?

mparker17’s picture

StatusFileSize
new3.22 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,754 pass(es).
[ View ]
new2.58 KB

I've tried to address the feedback from #23 in this patch.

Feedback welcome!

mparker17’s picture

Status:Needs work» Needs review

Whoops, Issue Status.

pwarn’s picture

The wording in #24 is much better.

Newbie idea/question, is there a way to link to the the local icon for contextual link inline with the word "pencil" If one were to see what the icon looked like in the context of the site they are viewing the help on, it might remove some confusion. Pictures being worth a thousand words and all that...

/core/misc/icons/whatever color code/pencil.svg

I don't know theming well enough to know if there is a way to link to the icon that makes sure that it's always the right color and gets properly overridden by an icon that a theme might use.

mparker17’s picture

Newbie idea/question, is there a way to link to the the local icon for contextual link inline with the word "pencil" If one were to see what the icon looked like in the context of the site they are viewing the help on, it might remove some confusion. Pictures being worth a thousand words and all that...

/core/misc/icons/whatever color code/pencil.svg

I don't know theming well enough to know if there is a way to link to the icon that makes sure that it's always the right color and gets properly overridden by an icon that a theme might use.

@pwarn there aren't any help pages that include images in that way. But, your idea would definitely be worth discussing in the Documentation Team group on groups.drupal.org!

jhodgdon’s picture

Status:Needs review» Needs work

Or just put it in and see how it works. You should be able to use the url() function or some kind of an image theming function to put in the link. Or there may be a theme hook that the contextual links module is using to make the buttons, that could be used, so that theme overrides would be respected? If not, I don't think it is horrible to show the Drupal Core default.

Anyway, I don't see a problem with having it embedded in the text, and a picture is definitely not a bad idea here. Sounds like a good idea, in fact -- someone want to make it happen?

In the meantime, I took a look at the latest patch and it all looks good except the very last line:

In either case, click the contextual links button for the content you want to edit, then click the task you want to perform.

It still says "for the content you want to edit". Ooops! Should be "...button for the area, then select the task you want to perform." I think?

minneapolisdan’s picture

Doing it now

minneapolisdan’s picture

StatusFileSize
new3.1 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,881 pass(es), 0 fail(s), and 3 exception(s).
[ View ]

Fixed a typo in #24, added pencil icon mentioned in #26, and reworded last sentence mentioned in #30.

jhodgdon’s picture

Please make an interdiff when adding a new patch to an issue that already has a patch: https://drupal.org/documentation/git/interdiff

minneapolisdan’s picture

StatusFileSize
new2.14 KB

Adding interdiff file for #30

minneapolisdan’s picture

Status:Needs work» Needs review

Change issue status

Status:Needs review» Needs work

The last submitted patch, 30: contextual-help-text-2091311-30.patch, failed testing.

jhodgdon’s picture

jhodgdon’s picture

Oh. This is not the right way to make a link to the pencil icon:

+      $output .= '<li>' . t('Hovering over the area where the content is displayed will temporarily make the button (which looks like a pencil in most themes, and is normally displayed in the upper right corner of the area) visible. The icon typically looks like this, <img src="!pencil" />', array('!pencil' => '/core/misc/icons/bebebe/pencil.svg', array('alt' => 'pencil icon'))) . '</li>';

The problem is you cannot assume that /core/misc/icons... is the right URL. The Drupal site's base URL need not be at / for instance (it could be example.com/sub/directory/siteroot for instance).

So you need to use the url() function to turn that path into a valid path.

I also think:
- The alt text should probably be "contextual links button" not "pencil icon".
- before the icon, we should have a : not a , in the text

The last submitted patch, 30: contextual-help-text-2091311-30.patch, failed testing.

minneapolisdan’s picture

Status:Needs work» Needs review
StatusFileSize
new3.11 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,946 pass(es), 0 fail(s), and 3 exception(s).
[ View ]
new2.15 KB

updated patch

minneapolisdan’s picture

StatusFileSize
new2.16 KB
new3.12 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,945 pass(es), 0 fail(s), and 3 exception(s).
[ View ]

Update patch with additonal text changes

The last submitted patch, 38: contextual-help-text-2091311-38.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 39: contextual-help-text-2091311-39.patch, failed testing.

jhodgdon’s picture

The help text here still has several text issues:

a) Under "Displaying contextual links", it just starts talking about the "contextual links button" without explaining what the button is for. This makes the text hard to follow. The text should start with something like "Contextual links for an area on a page are displayed using a contextual links button.", and then launch into the explanation of how to make this button visible.

b) It says that clicking the Toolbar button ...will make all contextual links on the page visible. This is incorrect. It actually makes all contextual links *buttons* on the page visible, not all the *links*.

c) As I am sure I have commented here several times:
In either case, click the contextual links button in the area you want to edit...
Do not talk about "the area you want to edit". This should probably say:

Once the contextual links button for the area of interest is visible, click the button ...

or something like that.

d)

+      $output .= '<li>' . t('Hovering over the area where the content is displayed will temporarily make the button (which looks like a pencil in most themes, and is normally displayed in the upper right corner of the area) visible. The icon typically looks like this: <img src="!pencil" />', array('!pencil' => url('core/misc/icons/bebebe/pencil.svg'), array('alt' => 'contextual links button'))) . '</li>';

This is not putting the alt text into the img tag, and the alt text needs to be translated in any case.

And I'm not sure where this htmlspecialcharacters error is happening, but it must have something to do with this patch? Did you try this patch out on a Drupal install to see if the image appeared?

benjf’s picture

Status:Needs work» Needs review
StatusFileSize
new3.17 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,187 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new2.58 KB

Updated patch and interdiff attached. Feedback welcome, of course (noob documentation contributor here).

Status:Needs review» Needs work

The last submitted patch, 43: contextual-help-text-2091311-43.patch, failed testing.

jhodgdon’s picture

This is an unrelated test failure -- people are looking into it.

Regarding the patch, it's pretty close! A couple of things to address:
a) The icon typically looks like this: <img src="!pencil" alt="!alt" />'
I am not sure why the alt text is put in here with a ! instead of directly in the text?

b) "Once the contextual links button for the area of interest is visible, click the button to display its links." ... I don't think "its" makes sense here -- whose links are we talking about? How about just "... to display the links"?

benjf’s picture

Status:Needs work» Needs review
StatusFileSize
new3.15 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,264 pass(es).
[ View ]
new1.95 KB

Thanks for the quick feedback, here's an updated patch.

jhodgdon’s picture

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

This looks great to me, thanks!

I'm going to mark it RTBC and move it into the Contextual Links module, in case the maintainers have any feedback. If not, I or one of the other core maintainers will commit it in a few days.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
+++ b/core/modules/contextual/contextual.module
@@ -69,11 +69,18 @@ function contextual_help($route_name, RouteMatchInterface $route_match) {
+      $output .= '<li>' . t('Hovering over the area of interest will temporarily make the contextual links button visible (which looks like a pencil in most themes, and is normally displayed in the upper right corner of the area). The icon typically looks like this: <img src="!pencil" alt="contextual links button" />', array('!pencil' => url('core/misc/icons/bebebe/pencil.svg'))) . '</li>';

Should be using \Drupal::url() not url()

er.pushpinderrana’s picture

Status:Needs work» Needs review
StatusFileSize
new1.84 KB
new3.16 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,499 pass(es), 47 fail(s), and 0 exception(s).
[ View ]

Should be using \Drupal::url() not url()

[DONE].

Please review updated patch.

Status:Needs review» Needs work

The last submitted patch, 49: contextual-help-text-2091311-49.patch, failed testing.

jhodgdon’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new3.15 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,778 pass(es).
[ View ]

#48 is incorrect. Drupal::url() expects a *route* and this is not a route, but a path to a file in the core installation. Using the url() function is correct in this case.

We need to go back to the patch in #46, so I am uploading it again.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work

Actually this should probably be file_create_url() ... sorry for the incorrect review.

jhodgdon’s picture

Ah, OK, yeah that looks like the right function to call.

er.pushpinderrana’s picture

Status:Needs work» Needs review
StatusFileSize
new3.16 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,805 pass(es).
[ View ]

Thanks alexpott and jhodgdon!

Please review updated patch, use file_create_url() instead of url().

+      $output .= '<li>' . t('Hovering over the area of interest will temporarily make the contextual links button visible (which looks like a pencil in most themes, and is normally displayed in the upper right corner of the area). The icon typically looks like this: <img src="!pencil" alt="contextual links button" />', array('!pencil' => file_create_url('core/misc/icons/bebebe/pencil.svg'))) . '</li>';

jhodgdon’s picture

Status:Needs review» Reviewed & tested by the community

Looks great! I applied this patch and tested it, and the pencil icon shows up fine. The rest of the help looks good too, and matches what I see in the UI. Thanks!

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed e24723a and pushed to 8.0.x. Thanks!

  • alexpott committed e24723a on 8.0.x
    Issue #2091311 by mparker17, minneapolisdan, benjf, er.pushpinderrana,...

Status:Fixed» Closed (fixed)

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