https://api.drupal.org/api/drupal/core%21modules%21views%21src%21Plugin%...

Current description is confusing.

Does the style plugin allows to use style plugins.

It should be changed to

To use Row plugins.

It appears that the description was copied to some derived plugins (Table).

CommentFileSizeAuthor
#18 2888689-18.patch4.64 KBpk188
#14 2888689-14.patch2.46 KBpk188
#12 2888689-12.patch2.56 KBpk188
#9 2888689-9.patch2.34 KBdhruveshdtripathi
#4 2888689-3.patch2.54 KBPavan B S
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Chi created an issue. See original summary.

Rishi Kulshreshtha’s picture

Rishi Kulshreshtha
he/him/his
Primary language English
CreditAttribution: Rishi Kulshreshtha as a volunteer commented
Assigned: Unassigned » Rishi Kulshreshtha
Rishi Kulshreshtha’s picture

Rishi Kulshreshtha
he/him/his
Primary language English
CreditAttribution: Rishi Kulshreshtha as a volunteer commented
Assigned: Rishi Kulshreshtha » Unassigned
Pavan B S’s picture

Pavan B S CreditAttribution: Pavan B S at Valuebound commented
Status: Active » Needs review
FileSize
2888689-3.patch2.54 KB

Applying the patch, please review and suggest me if i have entered a wrong description. I have taken StylePluginBase class in the description.

Chi’s picture

Chi CreditAttribution: Chi commented
Status: Needs review » Needs work

The issue not about spelling but meaning. The property controls row plugins.

Pavan B S’s picture

Pavan B S CreditAttribution: Pavan B S at Valuebound commented

@chi i didn't provide patch for spelling, i consider description based on StylePluginBase class. Sorry if I am wrong.

Chi’s picture

Chi CreditAttribution: Chi commented

@Pavan B S

to use style plugins

According to the property name it should be "to use row plugins".

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer commented
Issue summary: View changes

#7 +1
I have checked its usage. According to its usage and the property name, it should be "To use Row plugins"

Updated IS accordingly

dhruveshdtripathi’s picture

dhruveshdtripathi CreditAttribution: dhruveshdtripathi at DevsAdda commented
Status: Needs work » Needs review
FileSize
2888689-9.patch2.34 KB

Changes made according to comment #8.

Thank you.

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Thanks!

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented
Status: Reviewed & tested by the community » Needs work

Sorry but "To use Row plugins" does not sound like a proper English sentence to me. I would suggest something like "Whether or not this style uses a row plugin."

Also all other instances beside StylePluginBase should be changed to simply

/**
 * {@inheritdoc}
 */

So we don't have to duplicate the documentation.

pk188’s picture

pk188 CreditAttribution: pk188 at OpenSense Labs commented
Status: Needs work » Needs review
FileSize
2888689-12.patch2.56 KB

Agree with #11. Updated the patch accordingly.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented
Status: Needs review » Needs work

That's the direction I was talking about. Thanks @pk188! One minor problem, though, with the last patch:

+++ b/core/modules/views/src/Plugin/views/style/DefaultStyle.php
@@ -19,10 +19,12 @@
   /**
...
    * @var bool
    */

This should be removed as well (except for StylePluginBase).

pk188’s picture

pk188 CreditAttribution: pk188 at OpenSense Labs commented
Status: Needs work » Needs review
FileSize
2888689-14.patch2.46 KB

My mistake.
Fixed #13.

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer commented
Status: Needs review » Needs work

I have applied the patch and checked it locally. It still needs to apply the {@inheritdoc} to these child classes of StylePluginBase.

- Drupal\views\Plugin\views\style\Opml
- Drupal\views\Plugin\views\style\Rss
- Drupal\views_test_data\Plugin\views\style\StyleTemplateTest
- Drupal\views_test_data\Plugin\views\style\StyleTest

back to N/W for this

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented

Thanks @pk188, that's perfect.

I found some more plugins that need updating: Opml, Rss, StyleTemplateTest, StyleTest

I think we should update those too, then this is RTBC.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented

Oops, crosspost with @naveenvalecha in #15, sorry. But great that we came to the same conclusion ;-)

pk188’s picture

pk188 CreditAttribution: pk188 at OpenSense Labs commented
Status: Needs work » Needs review
FileSize
2888689-18.patch4.64 KB

Fixed #15.

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Thanks @pk188
Verified the latest patch on local and it addressed my concerns in #15

//Naveen

  • Gábor Hojtsy committed 91e5214 on 8.3.x 
    Issue #2888689 by pk188, Pavan B S, dhruveshdtripathi, naveenvalecha,...
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Version: 8.4.x-dev » 8.3.x-dev
Status: Reviewed & tested by the community » Fixed

Thanks, looks good to me too, committed.

  • Gábor Hojtsy committed 6fb6c6b on 8.4.x 
    Issue #2888689 by pk188, Pavan B S, dhruveshdtripathi, naveenvalecha,...
pk188’s picture

pk188 CreditAttribution: pk188 at OpenSense Labs commented

What's the status of issue. As @Gábor Hojtsy fixed and committed it, but it's status is still RTBC and no credit given to anybody.

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer commented

Please don't change the status of this issue. I have pinged the @mixologic over IRC to look into that so that he would be able to reproduce the issue and fix the issue on drupal.org

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 18: 2888689-18.patch, failed testing. View results

pk188’s picture

pk188 CreditAttribution: pk188 at OpenSense Labs commented
Status: Needs work » Reviewed & tested by the community

The issue has already fixed and committed. So that's why failed to apply.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Status: Reviewed & tested by the community » Fixed
pk188’s picture

pk188 CreditAttribution: pk188 at OpenSense Labs commented
Status: Fixed » Reviewed & tested by the community

@Gábor Hojtsy credit is not given to anyone, there is any reason for that? Can you please tell.

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer commented

I'm getting unnecessary notifications email just for the credit stuff which I really don't care. Ping Gabor over IRC regarding credit system(which I think not an issue if we'll not get the credit here)

P.S. :Unfollowing this issue as I don't feel there's anything else required to be done here.
//Naveen

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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