Skip to content
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

Fixes #27237 - List items in help with customization #309

Merged

Conversation

@ofedoren
Copy link
Member

ofedoren commented Jul 8, 2019

No description provided.

Copy link
Member

mbacovsky left a comment

I have mentioned some usability concerns inline. Maybe changes are not necessary.

# items - Array - array of items to be shown ([ ['Key', 'Value', { option: value }] ])
# options - Hash - same as #text
# block - optional block with custom modifications of each item in the list (must return modified item)
h.list(items, options = {}) { |item| ... }

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Aug 22, 2019

Member

I'm missing description of the {option: value} at the item level. From the code I'm seeing it can be bold:. Anything else? Also this is a bit confusing as it produces Key/Value list by default. Any examples how to format plain list of values properly indented?

Also what is the purpose of modification of the data in the block? Wouldn't it be more readable if we prepare the data before we try to format them? I'd see the benefit of the block if the items are expected to be list of strings and the block would do serialization of the items otherwise. e.g.

h.list(['a', 'b', 'c'])

or

h.list([['key', 'value', true], ['foo', 'bar', false]]) { |item| "#{item[2] ? bold(item[0]) : item[0] }: #{item[1]}" }

Am I missing something?

Command.extend_help do |h|
# Add a simple text to the help output.
# content - String - text to be shown
# options - Hash - options (id:, richtext:)

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Aug 22, 2019

Member

richtext: here means 'bold'?

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Aug 22, 2019

Member

by richtext I'd probably expect something like https://github.com/piotrmurach/tty-markdown ;) Didn't you consider it? Might be next PR candidate ;)

@ofedoren ofedoren force-pushed the ofedoren:bug-27237-customizable-list-items branch from 7c06391 to 74c9123 Aug 22, 2019
@ofedoren

This comment has been minimized.

Copy link
Member Author

ofedoren commented Aug 22, 2019

@mbacovsky, updated as per our discussion :)

h.note(content, options = {})
# Add a list of items to the help output.
# items - Array - array of items to be shown. Each item can be an array.
# Example: h.list(['a', 'b', 'c']) => "a b c\n"

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Aug 22, 2019

Member

shouldn't that be "a\n b\n c\n" ?

# options - Hash - options (same as #text, label:)
h.note(content, options = {})
# Add a list of items to the help output.
# items - Array - array of items to be shown. Each item can be an array.

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Aug 22, 2019

Member

Can we document also the item options? How about "Each item can be either string or an array of ['Key', 'Value', bold: true/false]. Item options are optional."

@ofedoren ofedoren force-pushed the ofedoren:bug-27237-customizable-list-items branch from 74c9123 to 2cc140b Aug 22, 2019
@ofedoren

This comment has been minimized.

Copy link
Member Author

ofedoren commented Aug 22, 2019

@mbacovsky, thanks! Updated.

Copy link
Member

mbacovsky left a comment

📜 Wunderbar! Thanks @ofedoren.

@mbacovsky mbacovsky merged commit 442ca13 into theforeman:master Aug 22, 2019
2 checks passed
2 checks passed
Redmine issues Valid issues
Details
hammer Build finished. 2400 tests run, 0 skipped, 0 failed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.