Support from Acquia helps fund testing for Drupal Acquia logo

Comments

czigor created an issue. See original summary.

czigor’s picture

Status: Active » Needs review
FileSize
1.25 KB
jhodgdon’s picture

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

Thanks for the issue and patch, and sorry it didn't get reviewed earlier -- I've been on vacation and hardly anyone else reviews documentation patches. :(

This needs some work though:

  1. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
    @@ -87,7 +87,7 @@
       /**
        * @var array
    -   * Stores additional fields which get's added to the query.
    +   * Stores additional fields which get added to the query.
        * The generated aliases are stored in $aliases.
        */
       var $additional_fields = array();
    

    Let's actually fix the doc block. It should start with a one-line summary.

    Then a blank line

    Then it can have additional paragraphs of explanation.

    Then at the end, the @var line.

  2. +++ b/core/modules/views/views.theme.inc
    @@ -547,8 +547,8 @@ function template_preprocess_views_view_table(&$variables) {
    +    // Remove columns if the option "hide empty column" is checked and the
    +    // field is empty.
         if (!empty($options['info'][$field]['empty_column'])) {
    

    Hm. It looks like the option is called "empty_column", not "hide empty column"?

czigor’s picture

@2: I meant the settings label in the UI which is "Hide empty column". Should I use the setting machine name or capitalize this one?

jhodgdon’s picture

Either way. :)

czigor’s picture

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

Status: Needs review » Fixed

Better, thanks! Committed to 8.0.x and 8.1.x, with small grammar fix of which->that.

  • jhodgdon committed 5089fed on 8.0.x
    Issue #2617822 by czigor: Comment fixes
    

Status: Fixed » Closed (fixed)

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