Skip to content

[Console] added TableCellStyle #37338

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 16, 2020

Conversation

khoptynskyi
Copy link
Contributor

@khoptynskyi khoptynskyi commented Jun 18, 2020

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #13370
License MIT
Doc PR symfony/symfony-docs#...

Added an opportunity to customize a table cell via TableCellStyle object.

It can be used as

new TableCell(
  'content',
  [
    'style' => new TableCellStyle([
        'align' => 'center',                            
        'fg' => 'red',
        'bg' => 'green',
        
        // or
        'cellFormat' => '<info>%s</info>',
    ])
  ]
)

See #13370

Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (minor comments)

$options = $this->getOptions();

return isset($options['align'])
&& self::DEFAULT_ALIGN !== $options['align'];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should on the same line


public function hasAlign(): bool
{
$options = $this->getOptions();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be inlined

return $this->options['style'];
}

public function hasAlign(): bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove this method

&& $style->hasAlign();
}

public function hasCellFormat(): bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove this method as well

{
$options = $this->getOptions();

return isset($options['cellFormat']) && \is_string($options['cellFormat']);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

@khoptynskyi khoptynskyi force-pushed the console-tablecellstyle branch 2 times, most recently from 700c4ba to dd70d46 Compare August 16, 2020 11:11
@khoptynskyi
Copy link
Contributor Author

@fabpot made edits

$rows[$line][$column] = new TableCell($lines[0], ['colspan' => $cell->getColspan()]);
$rows[$line][$column] = new TableCell(
$lines[0], ['colspan' => $cell->getColspan(), 'style' => $cell->getStyle()]
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move it on one line again?

$unmergedRows[$unmergedRowKey][$column] = new TableCell($value, ['colspan' => $cell->getColspan()]);
$unmergedRows[$unmergedRowKey][$column] = new TableCell(
$value, ['colspan' => $cell->getColspan(), 'style' => $cell->getStyle()]
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move it on one line again?

@@ -33,6 +34,10 @@ public function __construct(string $value = '', array $options = [])
throw new InvalidArgumentException(sprintf('The TableCell does not support the following options: \'%s\'.', implode('\', \'', $diff)));
}

if (isset($options['style']) && !$options['style'] instanceof TableCellStyle) {
throw new InvalidArgumentException('The style option must be instance of TableCellStyle.');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

must be an instance of

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"TableCellStyle"

}

/**
* @return array|string[]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@return string[] should be enough, no?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would that work?

@khoptynskyi
Copy link
Contributor Author

@fabpot done

return isset($this->getOptions()['align']) && self::DEFAULT_ALIGN !== $this->getOptions()['align'];
}

public function hasCellFormat(): bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for this one.

return $this->getOptions()['cellFormat'];
}

public function hasAlign(): bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not convinced that we should keep this method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the both methods can be deleted. Updated

}

/**
* @return array|string[]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would that work?

@fabpot fabpot force-pushed the console-tablecellstyle branch from 6b13b9f to aff7628 Compare August 16, 2020 13:33
@fabpot
Copy link
Member

fabpot commented Aug 16, 2020

Thank you @khoptynskyi.

@fabpot fabpot merged commit 03155ad into symfony:master Aug 16, 2020
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Aug 31, 2020
This PR was merged into the master branch.

Discussion
----------

[Console] added TableCellStyle documentation

See symfony/symfony#37338

Commits
-------

4a5be12 [Console] added TableCellStyle documentation
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
@fabpot fabpot mentioned this pull request Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Console][Table] Apply options/style to table cells
4 participants