I think we need this to fall in line with core modules. Title says it all.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

janip’s picture

Status: Active » Needs work
FileSize
809 bytes

An implementation of hook_help(). Good descriptions for use cases needed.

aspilicious’s picture

+++ b/core/modules/views/views_ui/views_ui.moduleundefined
@@ -13,6 +13,19 @@
+      $output .= '<p>' . t('The Views UI module provides <a href="@views-configuration">administration interface for Views</a>.', array('@views-configuration' => url('admin/structure/views'))) . '</p>';

The views UI module provides AN adminstration interface for Views

YesCT’s picture

Status: Needs work » Needs review

needs review to have the testbot run the patch

YesCT’s picture

Status: Needs review » Needs work
Issue tags: +Novice

and back to needs work to address #2

babruix’s picture

Status: Needs work » Needs review
FileSize
1.13 KB
YesCT’s picture

babruix’s picture

babruix’s picture

babruix’s picture

FileSize
154.98 KB

drupal-views-ui-help

Status: Needs review » Needs work
Issue tags: -Novice, -VDC

The last submitted patch, drupal-views-ui-help-1876906-8.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
Issue tags: +Novice, +VDC
YesCT’s picture

this doc about help will be helpful for whoever does a review: http://drupal.org/node/632280

dawehner’s picture

I like the simplicity of the current help. One additional idea: Would it make sense to somehow mentions the available tour here,
as it is a great learning starting point.

webchick’s picture

Priority: Normal » Critical

Missing docs are a critical.

tim.plunkett’s picture

Cross-referencing #1898794: Allow config entities to be exported. (Resolve regression from views in Drupal 7)., I think this issue should add some help text to the export page.

dawehner’s picture

Issue tags: -Novice, -VDC

Status: Needs review » Needs work
Issue tags: +Novice, +VDC

The last submitted patch, drupal-views-ui-help-1876906-8.patch, failed testing.

babruix’s picture

Status: Needs work » Needs review
FileSize
1.14 KB

Re-rolled last patch.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Seems legit.

webchick’s picture

Component: views_ui.module » documentation
Assigned: Unassigned » jhodgdon

Awesome, thanks for this! Moving to the docs queue for jhodgon to take a look.

damiankloip’s picture

Status: Reviewed & tested by the community » Needs work

Sorry....

+++ b/core/modules/views_ui/views_ui.module
@@ -13,6 +13,24 @@
+      $output .= '<p>' . t('The Views UI module provides the <a href="@views-configuration">administration interface</a> to create and edit your Views.',
+        array('@views-configuration' => url('admin/structure/views'))) . '</p>';

We don't need this link href to be run through check_plain? So we should be fine with a !views-configuration token instead?

Also, we should use the url generator now, instead of url? We can then just pass in the route name and not the path.

babruix’s picture

Status: Needs work » Needs review
FileSize
1.14 KB

I think url() is wrapper for Drupal::urlGenerator(): https://api.drupal.org/api/drupal/core!includes!common.inc/function/url/8
Token fixed.

damiankloip’s picture

Status: Needs review » Needs work

I think url() is wrapper for Drupal::urlGenerator()

Yes, but where possibly, especially in new patch, we should just use the service directly, the url() method mainly exists for bc/other places in core that were using this before. Also, url() uses the generateFormPath() method, which is deprecated anyway.

We can then just pass in the route name and not the path.

This is what you can't do with url(). We should use the UrlGenerator::generate() method and *not* (what essentially is) the generateFromPath() method here.

babruix’s picture

Status: Needs work » Needs review
FileSize
1.21 KB

Patch fixed using Drupal::urlGenerator() and $generator->generate('views_ui.list');

jhodgdon’s picture

Status: Needs review » Needs work

This is not following the standards we have for how to write hook_help() documentation. See the meta issue #1908570: [meta] Update or create hook_help() texts for D8 core modules for details and links. The standards are at https://drupal.org/node/632280

Also, the text that is in the Uses section is supposed to tell how to use the module. The text that is there now belongs in the About section, and should be reworded anyway.

The link that is in the About section, on the other hand, should probably be in the Uses section.

jhodgdon’s picture

So...

I have a question that is also pertinent for all of the other "write hook_help" issues on #1908570: [meta] Update or create hook_help() texts for D8 core modules and I'm bringing it up here because this is the first I've seen of a directive not to use url().

In many of the hook_help implementations, we need to reference help pages for other modules.

Currently, these are links made with url('admin/help/foo'), where 'foo' is the module name.

Looking at help.routing.yml, I see that the route for these pages is

help_page:
  pattern: 'admin/help/{name}'

So... not being an expert on this new routing system, what should I tell the mostly non-programmer documetnation people who are bravely making help documentation patches that they should put in here, if we are not supposed to use url() any more?

jhodgdon’s picture

If you could answer the above question on #1908570: [meta] Update or create hook_help() texts for D8 core modules that would be even better, since that is where the answer is needed.

jhodgdon’s picture

See #1908570-17: [meta] Update or create hook_help() texts for D8 core modules - I guess we want to use Drupal::url() now. Thanks for the quick response!!!

damiankloip’s picture

Yeah, I totally forgot we added the Drupal::url() wrapper around urlGenerator. Nice.

babruix’s picture

If someone has suggestions on what is missed or how patch could be improved - you are welcome!
Jennifer, what exactly you mean by saying "not following the standards we have for how to write hook_help"?
Maybe you have some examples?
Thanks.

batigolix’s picture

The standards for writing hook_help texts can be found here: https://drupal.org/node/632280 . It contains an example

Untill now we were using the following approach to add links to help texts:

$output .= '<dd>' . t('To configure a particular content type for translation, visit the <a href="!content-types">Content types</a> page, and click the <em>edit</em> link for the content type. In the <em>Publishing options</em> section, select <em>Enabled, with translation</em> under <em>Multilingual support</em>.', array('!content-types' => url('admin/structure/types'))) . '</dd>';

We are replacing url() now by \Drupal::url
See https://drupal.org/node/632280#url-note

babruix’s picture

Status: Needs work » Needs review
FileSize
1.29 KB

Okay, better initial version of hook_help implemented.

babruix’s picture

Added few improvements.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks!

So, there are a few things in this patch that are either non-standard or otherwise need a little work:

a) The URLs are not following https://drupal.org/node/632280#url-note [sorry, this changed since the comment above was posted. Not your fault but we need to change it again!]

b) "...From the admin/structure/views page..." -- This should instead be a link to that page, and the link text should be the name of that page.

c) It all needs some proofreeding/editing. Some of it is not grammatical.

d) In the About section, there should be a link to the Views module as well as to the on-line help on drupal.org for this module. https://drupal.org/node/632280 shows our standard help template format.

e) On the Views hook_help() page, it explains what a "view" is (no capital letter!). Here, they are called "Views". Pick one and be consistent.

f) On the Views issue I think we are removing some items there that belong on Views UI. Let's make sure all of them get added here. #1876904: Implement hook_help() for views.module

babruix’s picture

Status: Needs work » Needs review
FileSize
2.19 KB

Thanks for your comments, created new patch:
a) URLs fixed
b) link changed
c) text reviewed and fixed, but feel free to suggest improvements
d) link to Views help page added
e) fixed, please, review
f) done

Cyberschorsch’s picture

Status: Needs review » Needs work
FileSize
60.99 KB

This patch still needs a little bit of work. The second link has invalid HTML and outputs a false link (see screenshot attached).

Views UI Help Error

babruix’s picture

Status: Needs work » Needs review
FileSize
2.19 KB

Thanks, link corrected.

Cyberschorsch’s picture

Status: Needs review » Reviewed & tested by the community

Looks fine now :)

StuartJNCC’s picture

Status: Reviewed & tested by the community » Needs work

"versioned" should be "version" under Exporting and Importing.

In the Theming section, what does the second sentence: "Available list of currently used templates and list of suggestion names to override them." mean? It conveys nothing to me. Should it be a link to something?

babruix’s picture

Status: Needs work » Needs review
FileSize
2.18 KB

Changed last row, how it is now?

StuartJNCC’s picture

Much better!

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Status: Needs review » Needs work

This is mostly very good! A few things need to be fixed:

a) This file is not following the same format as all the other hook_help() files, which keep each line of docs on one line instead of splitting the t() calls. It makes the text a bit harder to scan in the patch.

b) Bug: at the top, it is setting $views_help to the URL for the Views UI help, but later on it is being used as if it is the help for the Views module, so this is incorrect.

c) Under "Editing" it says "It is possible to change title, change fields, filters, sortings.". This needs a bit of a rewrite:
- "sortings" is not actually a word
- There needs to be a "the" in there
- Probably only one word "change" is enough
- Probably needs to end in "etc.".

d) The Uses topic headings should use sentence case according to the template standards:
https://drupal.org/node/632280

e) I'd also like to see the Uses headings more consistent. They are currently:
Creating a View
Editing Views
Exporting and Importing
Theming

f) The Exporting/Importing and Theming items really don't give me any information on how to do either of these. Should they give a little more? I mean, exporting says you can put views in code but doesn't say how... is there some documentation that could be pointed to? Same with theming... it says it is possible to configure display output but it doesn't say where you would do that or how, and it says you can use custom templates but what it really should probably say is that you can override the templates in your theme and then tell you how to find out what templates you can override. Also probably the display settings should be a separate item from theming -- I think of Theming as having to do with my Theme, and the display settings are in the View, right?

mikemiles86’s picture

Assigned: Unassigned » mikemiles86
Issue summary: View changes

Updating patch #41 with suggestions from jhodgdon in #43.

mikemiles86’s picture

Assigned: mikemiles86 » Unassigned
Status: Needs work » Needs review
FileSize
3.32 KB

I have updated the help text created by babruix in #41, with changes suggested by jhodgdon in #43. I updated the help text and titles to be more inline with standards and formats used by other core modules, as well as made the descriptions more informative.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks! There are still several things that need to be fixed here...

We don't want to use the url() function any more. The standards for hook_help() are at: https://drupal.org/node/632280... it would be easier to review this patch if it more closely followed the template used by other hook_help() implementations instead of defining all the URLs at the top, and it would also be better if the writing style was more in line with the standard.

And there are also some specific issues with this text:

a) The About section definitely needs to have a link to the Views module help and mention the Views module.

b) We really don't need to say "with the appropriate permissions" all over the place. We don't do that in any of the other help files, and that's really just how Drupal works. Just take it out, or else replace by mentioning specific permissions. And we do not need it to link to the permissions page.

c) We shouldn't be saying "The Views UI module allows..." all over the place either. This is the help page for this module, so it's just a waste of space to say that everywhere.

d) Generally in help for one module we don't want to explain how to use other modules. So I wouldn't be specific in there about how to export and import views. I would just say they can be exported and imported as configuration using the whatever module and link to that module and leave it at that.

e) Typo:

used by a view, when editing a view, In the "Advanced" column,

In should not be capitalized.

mikemiles86’s picture

Thanks for reviewing jhodgdon.

Some questions/responses to the recent issues you've raised.

I had kept the placement of the generated urls at the top, since they are being reused in multiple places, so that \Drupal::url() was not called multiple times for the same url. However, I do see the point of keeping with standards.

Using the depreciated url() for a few links is due to my lack of understanding on how to use \Drupal::url() for more complicated links, such as the permissions one which has a fragment. Does \Drupal::url() work the same as url() ?

For a) Currently Views in D8 core does not have a help section (or at least I was unable to find a hook_help() in the .module ), so I thought that instead of linking to a none existing help section linking to the existing online handbook pages would be more accurate. Does the help section for views live somewhere else? or is there another issue for creating it?

For b/c) With how to write the help text I referenced the structure of other core modules such as user and menu, which both reference and link to their permission settings. Are we saying these core modules help sections are not written to the correct current standards?

For d) Definitely agree with you, the text should not be as specific describing how to export views. I was overcompensating for your f) in comment #43 where you said there was not enough information about that process.

For e) typo noted.

jhodgdon’s picture

Yes, you can pass the fragment to Drupal::url() -- works the same as url().
https://api.drupal.org/api/drupal/core!lib!Drupal.php/function/Drupal%3A...

a) Yes, there is a separate issue for adding help for Views itself. You can pretend it exists and make the link. I don't have the issue number handy.

b/c - If you want to talk about permissions, that is fine, but be specific and say which permission it is using. "appropriate permissions" doesn't really tell me anything useful, so if you don't want to be specific about permissions, take it out.

jhodgdon’s picture

mikemiles86’s picture

Assigned: Unassigned » mikemiles86

Updating help text base don recent feedback

mikemiles86’s picture

Status: Needs work » Needs review
FileSize
2.47 KB

Updated help text for Views UI based on latest feedback.

mikemiles86’s picture

Assigned: mikemiles86 » Unassigned
jhodgdon’s picture

Status: Needs review » Needs work

Thanks for the updated patch!

Sorry, I guess I wasn't clear in my earlier review. In the About, we want *both* a link to the Views module help and to the on-line documentation for Views UI. Probably the Views help link could go into the first sentence, something like "provides an interface for managing views for the (link)Views module(end of link).". And then we would have the standard help template's link to the on-line documentation in the second sentence.

The rest of the help text there looks great to me! The only part I thought was confusing was the last part about theming. That is probably because it is a confusing part of the UI... Let's see if we can make it better...

Hm...

Maybe:

The template files used by views can be overridden from a custom theme. When editing a view, you can see the templates that are used and alternatives for overriding them by clicking on the "Templates" link, found in the Advanced > Other section under "Output".

Or something like that?

Hm... This link is not working for me at the moment, but that is a separate problem:
#2154687: Templates link isn't working in Views edit (Exception: missing cache.theme service)
I'm just assuming the pop-up will give me alternatives like it does in D7 views.

mikemiles86’s picture

Assigned: Unassigned » mikemiles86
mikemiles86’s picture

Assigned: mikemiles86 » Unassigned
Status: Needs work » Needs review
FileSize
2.62 KB

Could this be the perfect one? Updated with recent feedback.

jhodgdon’s picture

jhodgdon’s picture

Well, almost!

About:
- It should be "Views module" not "Views Module".
- The online docs should go to the views UI module help page, not the Views module help page. If that page doesn't exist, we can create it.

The last line of Uses wasn't updated either. See #53.

mikemiles86’s picture

It's helpful to make sure you have saved your changes before making a patch.

And there is no online help page for Views UI, just for Views.

jhodgdon’s picture

There is now:
https://drupal.org/documentation/modules/views_ui
:)

So... When you create a new patch, can you please provide an interdiff file?
https://drupal.org/documentation/git/interdiff

This is looking good. I'm curious as to whether the > in the last line works, or does it need to be &gt;?

mikemiles86’s picture

Assigned: Unassigned » mikemiles86
Status: Needs review » Needs work

Working on discussed changes.

mikemiles86’s picture

Assigned: mikemiles86 » Unassigned
Status: Needs work » Needs review
FileSize
2.63 KB
2.22 KB

Update help text with latest feedback.

- linked to Views UI handbook
- replace > with >

interdiff between last two patches included.

jhodgdon’s picture

Status: Needs review » Needs work
Issue tags: +Needs manual testing

For link accessibility reasons, all links need to either have a title or have link text that tells you where you're going. So we've standardized the links to the online docs pages in hook_help() to have "online documentation for the Foo module" all inside the A tag, rather than just "Foo module" as you have here.

Other than that minor update, I think this looks great! It needs to be manually tested:
- Verify the formatting looks fine.
- Verify that all the links work.
- Verify that where UI text is mentioned (names of pages, labels on pages, link text, etc.), the help matches what you actually see on the UI page.

mikemiles86’s picture

Status: Needs work » Needs review
FileSize
2.63 KB
1.19 KB

Minor update for link text to online documentation for accessibility reasons.

jhodgdon’s picture

Thanks for the patch update! Ready for manual testing (see #62).

StuartJNCC’s picture

I think this needs a slight change in wording:

The Views UI module provides users an interface for managing views

"The Views UI module provides users with an interface for managing views" or maybe "The Views UI module provides a user interface for managing views".

damiankloip’s picture

#65, I think I like the latter one best.

mikemiles86’s picture

Updated with feedback from StuartJNCC, in #65.

jhodgdon’s picture

Rather than adding a word, how about removing one? We could say

The Views UI provides an interface ...

(leave "users" out entirely). This would be more in line with other help text.

jhodgdon’s picture

Status: Needs review » Needs work
tarekdj’s picture

Status: Needs work » Needs review
FileSize
2.62 KB
1.26 KB

user removed

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

I think this is ready to be committed. Thanks to all who worked on this patch!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Great work, everyone!

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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