Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
I think we need this to fall in line with core modules. Title says it all.
Comment | File | Size | Author |
---|---|---|---|
#70 | interdiff.txt | 1.26 KB | tarekdj |
#70 | drupal-views-ui-help-1876906-70.patch | 2.62 KB | tarekdj |
#67 | interdiff-63-67.txt | 1.19 KB | mikemiles86 |
#67 | drupal-views-ui-help-1876906-67.patch | 2.63 KB | mikemiles86 |
#63 | interdiff-61-63.txt | 1.19 KB | mikemiles86 |
Comments
Comment #1
janip CreditAttribution: janip commentedAn implementation of hook_help(). Good descriptions for use cases needed.
Comment #2
aspilicious CreditAttribution: aspilicious commentedThe views UI module provides AN adminstration interface for Views
Comment #3
YesCT CreditAttribution: YesCT commentedneeds review to have the testbot run the patch
Comment #4
YesCT CreditAttribution: YesCT commentedand back to needs work to address #2
Comment #5
babruix CreditAttribution: babruix commentedComment #6
YesCT CreditAttribution: YesCT commentedrelated #1876904: Implement hook_help() for views.module
Comment #7
babruix CreditAttribution: babruix commentedComment #8
babruix CreditAttribution: babruix commentedComment #9
babruix CreditAttribution: babruix commentedComment #11
damiankloip CreditAttribution: damiankloip commented#8: drupal-views-ui-help-1876906-8.patch queued for re-testing.
Comment #12
YesCT CreditAttribution: YesCT commentedthis doc about help will be helpful for whoever does a review: http://drupal.org/node/632280
Comment #13
dawehnerI 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.
Comment #14
webchickMissing docs are a critical.
Comment #15
tim.plunkettCross-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.
Comment #16
dawehner#8: drupal-views-ui-help-1876906-8.patch queued for re-testing.
Comment #18
babruix CreditAttribution: babruix commentedRe-rolled last patch.
Comment #19
dawehnerSeems legit.
Comment #20
webchickAwesome, thanks for this! Moving to the docs queue for jhodgon to take a look.
Comment #21
damiankloip CreditAttribution: damiankloip commentedSorry....
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.
Comment #22
babruix CreditAttribution: babruix commentedI think url() is wrapper for Drupal::urlGenerator(): https://api.drupal.org/api/drupal/core!includes!common.inc/function/url/8
Token fixed.
Comment #23
damiankloip CreditAttribution: damiankloip commentedYes, 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.
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.
Comment #24
babruix CreditAttribution: babruix commentedPatch fixed using Drupal::urlGenerator() and $generator->generate('views_ui.list');
Comment #25
jhodgdonThis 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.
Comment #26
jhodgdonSo...
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
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?
Comment #27
jhodgdonIf 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.
Comment #29
jhodgdonSee #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!!!
Comment #30
damiankloip CreditAttribution: damiankloip commentedYeah, I totally forgot we added the Drupal::url() wrapper around urlGenerator. Nice.
Comment #31
babruix CreditAttribution: babruix commentedIf 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.
Comment #32
batigolixThe 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:
We are replacing url() now by \Drupal::url
See https://drupal.org/node/632280#url-note
Comment #33
babruix CreditAttribution: babruix commentedOkay, better initial version of hook_help implemented.
Comment #34
babruix CreditAttribution: babruix commentedAdded few improvements.
Comment #35
jhodgdonThanks!
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
Comment #36
babruix CreditAttribution: babruix commentedThanks 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
Comment #37
CyberschorschThis patch still needs a little bit of work. The second link has invalid HTML and outputs a false link (see screenshot attached).
Comment #38
babruix CreditAttribution: babruix commentedThanks, link corrected.
Comment #39
CyberschorschLooks fine now :)
Comment #40
StuartJNCC CreditAttribution: StuartJNCC commented"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?
Comment #41
babruix CreditAttribution: babruix commentedChanged last row, how it is now?
Comment #42
StuartJNCC CreditAttribution: StuartJNCC commentedMuch better!
Comment #43
jhodgdonThis 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?
Comment #44
mikemiles86Updating patch #41 with suggestions from jhodgdon in #43.
Comment #45
mikemiles86I 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.
Comment #46
jhodgdonThanks! 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:
In should not be capitalized.
Comment #47
mikemiles86Thanks 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.
Comment #48
jhodgdonYes, 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.
Comment #49
jhodgdonViews now has help: #1876904: Implement hook_help() for views.module. :)
Comment #50
mikemiles86Updating help text base don recent feedback
Comment #51
mikemiles86Updated help text for Views UI based on latest feedback.
Comment #52
mikemiles86Comment #53
jhodgdonThanks 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.
Comment #54
mikemiles86Comment #55
mikemiles86Could this be the perfect one? Updated with recent feedback.
Comment #56
jhodgdonComment #57
jhodgdonWell, 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.
Comment #58
mikemiles86It'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.
Comment #59
jhodgdonThere 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
>
?Comment #60
mikemiles86Working on discussed changes.
Comment #61
mikemiles86Update help text with latest feedback.
- linked to Views UI handbook
- replace > with >
interdiff between last two patches included.
Comment #62
jhodgdonFor 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.
Comment #63
mikemiles86Minor update for link text to online documentation for accessibility reasons.
Comment #64
jhodgdonThanks for the patch update! Ready for manual testing (see #62).
Comment #65
StuartJNCC CreditAttribution: StuartJNCC commentedI think this needs a slight change in wording:
"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".
Comment #66
damiankloip CreditAttribution: damiankloip commented#65, I think I like the latter one best.
Comment #67
mikemiles86Updated with feedback from StuartJNCC, in #65.
Comment #68
jhodgdonRather 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.
Comment #69
jhodgdonComment #70
tarekdj CreditAttribution: tarekdj commenteduser removed
Comment #71
jhodgdonI think this is ready to be committed. Thanks to all who worked on this patch!
Comment #72
webchickGreat work, everyone!
Committed and pushed to 8.x. Thanks!