Posted by thsutton on February 9, 2013 at 5:55am
9 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | documentation |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | #SprintWeekend, documentation, Novice, VDC |
Issue Summary
Add documentation comments to the views pager plugins as described in #1882558: [META] Document all views plugins types.
Comments
#1
Add details to the documentation block for
PagerPluginBase.#2
Tagging as for parent issue.
#3
Great!!
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/pager/PagerPluginBase.phpundefined@@ -21,6 +21,32 @@
+ * To define a pager type, extend this base class. The ViewsPluginManager (used to
+ * create views plugins objects) adds annotated discovery for block plugins. Your
Ups, the same odd sentence, so yeah it should be named 'for pager plugins'.
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/pager/PagerPluginBase.phpundefined@@ -21,6 +21,32 @@
+ * - short_title: The "short" title for your pager type; this is displayed in
I guess you could add the "this is displayed in the views UI" in the access issue as well.
#4
The last submitted patch, document-views-pager-base-plugin-1912650-1.patch, failed testing.
#5
#1: document-views-pager-base-plugin-1912650-1.patch queued for re-testing.
#6
The last submitted patch, document-views-pager-base-plugin-1912650-1.patch, failed testing.
#7
It would be nice if the description for the full title explained where it is used.
+ * - title: The "full" title for your pager type; the short_title is used in
+ * the views UI when specified.
Information about the short title does not belong in the title description. It should go under the short title notes.
#8
#9
Cool, that you are working on that.
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/pager/PagerPluginBase.phpundefined@@ -21,6 +21,32 @@
+ * - type: The string "basic".
Is type really supported by pagers?
#10
Good spot!
We haven`t found any instances of type there, so decided to remove it, attaching new patch.
#11
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/pager/PagerPluginBase.phpundefined@@ -21,6 +21,31 @@
+ * - title: The "full" title for your pager type; used in the views UI;
+ * - short_title: The "short" title for your pager type;
+ * the short_title is used in the views UI when specified;
Last minor nitpick: this should have a dot instead of a "." at the end. Sorry.
#12
Ok finally changed ";" at the line ends to "."
#13
Great!
#14
The last submitted patch, drupal8.views-document-views-pager-base-plugin-1912650-12.patch, failed testing.
#15
#12: drupal8.views-document-views-pager-base-plugin-1912650-12.patch queued for re-testing.
#16
The last submitted patch, drupal8.views-document-views-pager-base-plugin-1912650-12.patch, failed testing.
#17
Requeued test manually on bot.
#18
It's just
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/pager/PagerPluginBase.phpundefined@@ -21,6 +21,31 @@
+ * @Plugin(
+ * id = "demo_pager",
+ * title = @Translation("Display a demonstration pager"),
+ * help = @Translation("Demonstrate pagination of views items."),
+ * theme = "views_demo_pager"
This should be wrapped in @code and @endcode, sorry i haven't seen that before.
#19
Fixed.
#20
The last submitted patch, drupal8.views-document-views-pager-base-plugin-1912650-19.patch, failed testing.
#21
Hmm, Is something wrong with my patch or with environment?
Log says
'Installing: failed to complete installation by setting admin username/password/etc.'which is not very helpful...#22
#19: drupal8.views-document-views-pager-base-plugin-1912650-19.patch queued for re-testing.
#23
The last submitted patch, drupal8.views-document-views-pager-base-plugin-1912650-19.patch, failed testing.
#24
The problem is indeed that you have "@Plugin" in there. One thing you can do is to use "@ Plugin" and mention the space in there.
#25
Thanks! Corrected.
#26
The last submitted patch, drupal8.views-document-views-pager-base-plugin-1912650-25.patch, failed testing.
#27
Also added space in "@ Translation" - looks it tries to import annotation if some text exists is after "@" symbol.
#28
We don't seem to have a better solution at the moment.
#29
This looks good to me, but assigning to Jennifer since it's docs-related.
#30
Um. The first paragraph says: "...including getting setting the..." -- that can't be quite right? Is it getting, setting, or does it need an "and" in there?
In the second paragraph, I'm also wondering about the word "should". Is the @Plugin mandatory or optional? If it's mandatory, let's say "must" there. If it's optional, ... what would the alternative be? And I think the text should also explain that in your actual code, you need to remove the spaces after the @ signs.
Same concern with the "should" immediately following the example. Leave that out (maybe say "The definition uses the following keys" instead?) and put (optional) in list items that are optional. See http://drupal.org/coding-standards/docs#lists for list syntax.
And then for the "short" item, it says:
+ * - short_title: The "short" title for your pager type.+ * The short_title is used in the views UI when specified.
+ * This is displayed in the views UI.
This seems to be a bit redundant? And the second (and third, if you don't remove it) lines need to be wrapped along with the first to as close to 80 characters as possible.
#31
Oh, forgot to say: I think the way you did the @code is fine and generally the docs look good -- I just think that the spaces after the @ signs need to be explained so that people know they need to remove them in their actual plugins.
#32
Changed patch following your comments.
#33
The first paragraph is still maybe missing an "and" before the last comma, or maybe an "etc." at the end?? It says:
Pager plugins take care of everything regarding pagers, including getting and setting the total number of items to render the pager, setting the global pager arrays.
And it should all be wrapped to 80 characters, as close as possible.
Then below the code it says:
+ * Remove spaces after @ in your plugin - these are needed only to pass tests.Umm... to pass tests? I would maybe say "these are put into this sample code so that it is not recognized as annotation" or something like that? Also, you might mention it has to go at the very end of the documentation block (at least I think that is true).
Other than that, looks great!
#34
Changed all as you`ve suggested, only I`m not sure that note to remove spaces has to go at the very end of the documentation block - because for developers it will be very easy to miss it.
#35
This looks OK to me... if you think the note about removing spaces should go elsewhere, feel free to move it. And maybe in the last bit we should say "The plugin annotation contains these components" instead of "The definition uses the following keys"? When I read that today, I was like... what definition are we talking about here all of a sudden? It's never referred to above as a definition.
#36
#37
That looks good to me, thanks!
I'd like someone from the Views project to review the documentation to make sure it's accurate before we commit it though (there was a review in #28 but since then the text and designation of what is optional has changed a bit).
#38
It's a general disadvantage of the concept of plugins compared to hooks, that we had example code in the .api.php file, though we don't have a place yet for example plugins. Just wondering whether there should be an issue for that.
Beside from that the documentation look pretty good.
#39
On api.drupal.org, and I would think in an IDE as well, you should be able to figure out what classes in Core extend a given plugin class, and presumably those core classes would be good examples? If more is needed than that, let's start a separate issue to discuss how to provide examples and/or how to locate Core examples.
Anyway, meanwhile, the patch in #36 still needs a review for documentation accuracy.
#40
Oh good point! I think this documentation is great.
#41
patch successfully applied for latest build, no grammatical errors.
+1
#42
OK then. :)
#43
Committed to 8.x. Thanks all!
#44
Automatically closed -- issue fixed for 2 weeks with no activity.