bedrock deployment get minor improvements

microsoft / bedrock-cli

The CLI for Bedrock

https://microsoft.github.io/bedrock-cli/commands/

MIT License

28 stars 12 forks source link

bedrock deployment get minor improvements #38

Closed samiyaakhtar closed 4 years ago

samiyaakhtar commented 4 years ago

Default top deployments to 50, to match default in spektate dashboard
A flag to override having no separators in the table view, but show separators by default
Introduce state column to normal mode but remove start time column, and also added duration column
Truncated some fields on normal mode to adjust the addition of new columns, but no truncation in wide mode
Introduced author to be shown in wide output, since it adds api calls
Refactored some messy table printing code in get.ts and moved it to lib/deploymenttable.ts

See comments for updated screenshots

samiyaakhtar commented 4 years ago

you have added two new options --top and --remove-separators and we do not see unit tests for them.

@dennisseah There are unit tests for these, but they're not at command level (just like for all other flags)

dennisseah commented 4 years ago

you have added two new options --top and --remove-separators and we do not see unit tests for them.

@dennisseah There are unit tests for these, but they're not at command level (just like for all other flags)

how can we test if 10 entries are returned if we do --top 50? what about negative test? --top x or --top -1. and the show and hide separators, are separators shown or hidden respectively?

gemorris commented 4 years ago

I am confused about the following item:

Introduce state column to normal mode but remove start time column, and also added duration column

Why was the start time column removed? Did we have user feedback about this? My concern is that the rows are now disconnected from time (It looks like we have end time in the wide version though?)

Would like to understand the thinking here.

samiyaakhtar commented 4 years ago

@gemorris From our last discussion about this a while ago, we decided that the real estate being used by start time could be utilized better, in both wide and narrow outputs, so we had decided to bring in author, PR and merged by to utilize that real estate. I could bring back start time but the time strings are long. Let me know if it should be brought back :)

samiyaakhtar commented 4 years ago

A few minor improvements:

All cells are left aligned except for numbers
Dashes were being used as fillers, removing them as they create noise, now that we have an explanation for empty cells ("Manual HLD Edit")
Introduced start time in wide column instead of end time, since it may add more value than end time

Screen Shot 2020-05-06 at 12 18 23 PM

Let me know your feedback @gemorris @andrebriggs @timfpark

samiyaakhtar commented 4 years ago

@andrebriggs @gemorris As discussed, UX for Manual HLD Edit and support for csv output formats will come in a later PR (see here). Right aligned Duration column.

Please give feedback so we can close this out :)

Screen Shot 2020-05-08 at 12 17 09 PM

andrebriggs commented 4 years ago

@samiyaakhtar so you're saying you want to show the HLD only edits by default for now?

samiyaakhtar commented 4 years ago

@andrebriggs For now, yes, until we work on the item Flag for manual hld edits, disabled by default, so that the default display is just deployments that are bedrock initiated in https://github.com/microsoft/bedrock/issues/743

The other reason is that it would be nice to explore colors and custom fonts in this library as part of a separate PR effort, so that "Manual HLD Edit" is shown in a more user friendly manner