Support from Acquia helps fund testing for Drupal Acquia logo

Comments

thsutton’s picture

Add details to the documentation block for PagerPluginBase.

thsutton’s picture

Issue tags: +Documentation, +Novice, +VDC

Tagging as for parent issue.

dawehner’s picture

Status: Active » Needs review

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.

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

The last submitted patch, document-views-pager-base-plugin-1912650-1.patch, failed testing.

samhassell’s picture

Status: Needs work » Needs review

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

The last submitted patch, document-views-pager-base-plugin-1912650-1.patch, failed testing.

mitron’s picture

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.

babruix’s picture

Status: Needs work » Needs review
Issue tags: +SprintWeekend2013
FileSize
2.01 KB
dawehner’s picture

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?

babruix’s picture

Good spot!
We haven`t found any instances of type there, so decided to remove it, attaching new patch.

dawehner’s picture

+++ 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.

babruix’s picture

Ok finally changed ";" at the line ends to "."

dawehner’s picture

Assigned: thsutton » Unassigned
Status: Needs review » Reviewed & tested by the community

Great!

Status: Reviewed & tested by the community » Needs work
Issue tags: -Documentation, -Novice, -VDC, -SprintWeekend2013

The last submitted patch, drupal8.views-document-views-pager-base-plugin-1912650-12.patch, failed testing.

babruix’s picture

Status: Needs work » Needs review

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

The last submitted patch, drupal8.views-document-views-pager-base-plugin-1912650-12.patch, failed testing.

jthorson’s picture

Requeued test manually on bot.

dawehner’s picture

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.

babruix’s picture

Status: Needs work » Needs review
FileSize
2.02 KB

Fixed.

Status: Needs review » Needs work

The last submitted patch, drupal8.views-document-views-pager-base-plugin-1912650-19.patch, failed testing.

babruix’s picture

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...

dawehner’s picture

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

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

The last submitted patch, drupal8.views-document-views-pager-base-plugin-1912650-19.patch, failed testing.

dawehner’s picture

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.

babruix’s picture

Status: Needs work » Needs review
FileSize
2.02 KB

Thanks! Corrected.

Status: Needs review » Needs work

The last submitted patch, drupal8.views-document-views-pager-base-plugin-1912650-25.patch, failed testing.

babruix’s picture

Status: Needs work » Needs review
FileSize
2.02 KB

Also added space in "@ Translation" - looks it tries to import annotation if some text exists is after "@" symbol.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

We don't seem to have a better solution at the moment.

webchick’s picture

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

This looks good to me, but assigning to Jennifer since it's docs-related.

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

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.

jhodgdon’s picture

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.

babruix’s picture

Status: Needs work » Needs review
FileSize
2.05 KB

Changed patch following your comments.

jhodgdon’s picture

Status: Needs review » Needs work

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!

babruix’s picture

Status: Needs work » Needs review
FileSize
2.11 KB

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.

jhodgdon’s picture

Status: Needs review » Needs work

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.

babruix’s picture

Status: Needs work » Needs review
FileSize
2.12 KB
jhodgdon’s picture

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).

dawehner’s picture

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.

jhodgdon’s picture

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.

dawehner’s picture

Oh good point! I think this documentation is great.

oresh’s picture

patch successfully applied for latest build, no grammatical errors.
+1

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

OK then. :)

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks all!

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