Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Console] Add title table#26933
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| TABLE | ||
| ), | ||
| array( | ||
| 'Boooooooooooooooooooooooooooooooooooooooooooooooooooooooooks', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
what about truncating instead of ignoring?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Indeed, it's better. Done.
| * Example: <code>+-----+-----------+-------+</code> | ||
| */ | ||
| privatefunctionrenderRowSeparator(int$type =self::SEPARATOR_MID) | ||
| privatefunctionrenderRowSeparator(int$type =self::SEPARATOR_MID,$title =null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
string $title = null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done.
| return$this; | ||
| } | ||
| publicfunctionsetTitle($title):self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
?string $title?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done.
| 'Boooooooooooooooooooooooooooooooooooooooooooooooooooooooooks', | ||
| 'default', | ||
| <<<'TABLE' | ||
| + Booooooooooooooooooooooooooooooooooooooooooooooooooooooo... + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What if we leave at least 1 separator character for long titles in the head? Instead of this:
+ Booooooooooooooooooooooooooooooooooooooooooooooooooooooo... +| ISBN | Title | Author |+---------------+--------------------------+------------------+| 99921-58-10-7 | Divine Comedy | Dante Alighieri || ...This:
+- Booooooooooooooooooooooooooooooooooooooooooooooooooooo... -+| ISBN | Title | Author |+---------------+--------------------------+------------------+| 99921-58-10-7 | Divine Comedy | Dante Alighieri || ...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done. That looks much better!
| } | ||
| /** | ||
| * Returns the text within a portion of a string, using mb_substr if it is available. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We could remove... using mb_substr if it is available from the description because it's an internal detail and in the PHPdoc we usually only explain what we do but not how we do that.
Also, the description is a bit confusingReturns the text within a portion of a string ... because the function name meansReplace a part of the given string...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I had put themb_substr detail to be consistent with the others methods of this helper, but effectively, that isn't really useful, so it's deleted now.
About the description, I reworded toReplaces text within a portion of a string (the same description than in thePHP doc)
| } | ||
| if (null !==$title) { | ||
| $lenT = Helper::strlenWithoutDecoration($formatter =$this->output->getFormatter(),$formattedTitle =sprintf($format =$this->style->getTitleFormat(),$title)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Let's use better variable names here to ease future maintenance. For example:
$lenT ->$titleTextLength$lenM ->$titleMarkupLength
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Renamed :)
30a72f8 to47183edComparefabpot commentedApr 16, 2018
Would it make sense to allow the title to be at the bottom as well? |
maidmaid commentedApr 16, 2018
@fabpot What do you mean by "at the bottom"? The second line of the header or the very last line of the table? |
fabpot commentedApr 16, 2018
I was thinking about the last line of the table. |
maidmaid commentedApr 18, 2018
IMO, shown the title only in the first line is enough. |
fabpot commentedApr 19, 2018
Not sure I understand the "enough" part. I was thinking about letting the user choose between displaying the title at the top or at the bottom (depending on what you want to achieve, doing one or the other might make more sense). |
ro0NL commentedApr 19, 2018
What about |
yceruto commentedApr 19, 2018
Also sometimes the table is too big, showing the title in both places (top/bottom) would be nice, avoids scrolling. |
maidmaid commentedApr 24, 2018
As suggested, I added
|
| return$this; | ||
| } | ||
| publicfunctionsetfooterTitle(?string$title):self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
s/footer/Footer/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done
| return$this; | ||
| } | ||
| publicfunctionsetfooterTitle(?string$title):self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
method should be renamed tosetFooterTitle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done
| /** | ||
| * Replaces text within a portion of a string. | ||
| */ | ||
| publicstaticfunctionsubstr_replace(string$string,string$replacement,int$start,int$length =null):string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not sure this actually complies with CS ... should we prefersubstrReplace() (givenstrlenWithoutDecoration also)
but i understand the reasoning.. i don't mind really :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe, someone like@nicolas-grekas could help us here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
the logic is used once: let's inline it instead of creating a new public method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Inlined!
| returnsubstr_replace($string,$replacement,$start,$length); | ||
| } | ||
| returnmb_substr($string,0,$start).$replacement.mb_substr($string,$start +$length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fromhttp://php.net/manual/en/function.substr-replace.php
i think we should default$length = strlen($string); here as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That looks working as expectedhttps://3v4l.org/C5Nhs, do I miss something ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Seems like PHP treatsnull asint(0) 🤔
| * Helper is the base class for all helper classes. | ||
| * | ||
| * @author Fabien Potencier <fabien@symfony.com> | ||
| * @author Dany Maillard <danymaillard93b@gmail.com> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
can be removed now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oops, I just removed it.
fabpot commentedJul 11, 2018
Thank you@maidmaid. |
This PR was merged into the 4.2-dev branch.Discussion----------[Console] Add title table| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | /| License | MIT| Doc PR | /Why not just add a title to console tables? Inspired by the famous file manager [Midnight Commander](https://en.wikipedia.org/wiki/Midnight_Commander).Commits-------6bf9eeb Add title table
javiereguiluz commentedSep 7, 2018
I've createdsymfony/symfony-docs#10294 to not forget about adding the docs for this feature.@maidmaid it'd be great if you could help us with the docs for this feature. Thanks! |
Why not just add a title to console tables? Inspired by the famous file managerMidnight Commander.