Skip to content
This repository has been archived by the owner on Feb 1, 2022. It is now read-only.

feat: add title to tables and improve output structure #364

Merged
merged 4 commits into from
Jun 9, 2021

Conversation

RodEsp
Copy link
Contributor

@RodEsp RodEsp commented May 25, 2021

What does this PR do?

Makes the table output more readable and allows adding a title with a divider to a table.

Before

image

After

image

GUS Work Item

@W-9103917@

@RodEsp RodEsp requested review from amphro, RasPhilCo and mdonnalley May 25, 2021 21:06
@RodEsp RodEsp self-assigned this May 25, 2021
@RodEsp
Copy link
Contributor Author

RodEsp commented May 25, 2021

Not sure if this would be a breaking change or not, but considering it's just human readable output I think a minor version bump would be fine. Thoughts @oclif/admins?

Copy link
Member

@mshanemc mshanemc left a comment

Choose a reason for hiding this comment

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

a lot of sfdx-cli's existing tables use the styledHeader

this.ux.styledHeader(Users in org ${this.org.getOrgId()});

suggestion: bold the header like that does (but don't use the === stuff since there's already plenty of borders in this PR.

then maybe that sets the header off enough visually that you don't need the row of === that this PR adds?

@RodEsp
Copy link
Contributor Author

RodEsp commented May 25, 2021

@mshanemc yeah, title is not required so if you don't pass it in but do something like

cli.styledHeader('blah')
cli.table(data, columns, options);

You get this
image

The ================ divider comes directly from the design for the Unified CLI as seen here.

@mshanemc
Copy link
Member

@mshanemc yeah, title is not required so if you don't pass it in but do something like

cli.styledHeader('blah')
cli.table(data, columns, options);

You get this
image

The ================ divider comes directly from the design for the Unified CLI as seen here.

I like yours better with the solid line down the side. Maybe that's just the font but it looks good.

for (const col of columns) {
const header = col.header!
headers += header.padEnd(col.width!)
}
options.printLine(chalk.bold(headers))

// print header dividers
let dividers = '| '
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the | devider also be between all columns? SHould both of these be configurable?

Copy link
Contributor Author

@RodEsp RodEsp May 26, 2021

Choose a reason for hiding this comment

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

@amphro Geordie brought that up during one of the previous design sessions and we basically all said in the name of saving space they weren't needed between columns. That's why they don't show up in the env command spec.

I could make them configurable but I don't think it's necessary since it's a straight up improvement over the old format that was harder to read and it's a human readable format so we're not breaking (or shouldn't be) things for people.

@RodEsp
Copy link
Contributor Author

RodEsp commented May 26, 2021

@mshanemc yeah that's definitely just my font. I use The Fira Code Nerd Font with ligatures for coding & my terminal.

@RodEsp
Copy link
Contributor Author

RodEsp commented Jun 1, 2021

@oclif/admins any more thoughts/QA on this before we merge?

@RodEsp
Copy link
Contributor Author

RodEsp commented Jun 3, 2021

Just made a new change to remove the | line on the right when there is no title.

So when you include a title you get something like the top table in the below screenshot and when you don't you get something like the bottom table.
image

@mdonnalley mdonnalley merged commit 46f1c18 into master Jun 9, 2021
@mdonnalley mdonnalley deleted the re/more_readable_table branch June 9, 2021 17:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants