WIP: add compact flag #83

Open
caninodev wants to merge 1 commits from caninodev/cview:compact_list into master
caninodev commented 1 year ago

Add an option to render the list compactly (without blank lines between ListItems). This flag will be ignored if showSecondaryText is set to true.

Add an option to render the list compactly (without blank lines between `ListItem`s). This flag will be ignored if `showSecondaryText` is set to true.
caninodev added 1 commit 1 year ago
28827c1146 add compact flag
tslocum requested changes 1 year ago
tslocum left a comment
Owner

Thanks for submitting this! I really appreciate it. I have a few ideas and some feedback on the implementation. Let me know what you think.

Thanks for submitting this! I really appreciate it. I have a few ideas and some feedback on the implementation. Let me know what you think.
// The height of the list the last time it was drawn.
height int
// If true, will render each ListItem without blank lines in between. This flag
tslocum commented 1 year ago
Owner

Could I suggest widening the scope of this feature slightly? A compact list could be a list which, when no list items have secondary text set, will not add a blank line for the missing text, and will render as though ShowSecondaryText were false. When an item with secondary text is added, it is as though ShowSecondaryText were set to true. I also believe compact should default to true.

Could I suggest widening the scope of this feature slightly? A compact list could be a list which, when no list items have secondary text set, will not add a blank line for the missing text, and will render as though `ShowSecondaryText` were false. When an item with secondary text is added, it is as though `ShowSecondaryText` were set to true. I also believe compact should default to true.
Poster

So to make sure I understand your suggestion:

If a list's list items do not have SecondaryText defined, then the list should be rendered compact by default.

However, should a list's list items have SecondaryText defined, then list should not be rendered compact by default?

Or do you mean to say that the list, in both conditions, be rendered compact?

So to make sure I understand your suggestion: If a list's list items do not have `SecondaryText` defined, then the list should be rendered compact by default. However, should a list's list items have `SecondaryText` defined, then list should _not_ be rendered compact by default? Or do you mean to say that the list, in both conditions, be rendered compact?
tslocum commented 1 year ago
Owner

The prior is what I meant. Thanks!

The prior is what I meant. Thanks!
// SetCompactList sets the flag that determines whether a blank line is drawn between
// each ListItem. This flag will be ignored if `showSecondaryText` is true.
func (l *List) SetCompactList(compact bool) {
tslocum commented 1 year ago
Owner

Please rename as SetCompact

Please rename as `SetCompact`
} else {
if l.currentItem-l.itemOffset >= h {
l.itemOffset = l.currentItem + 1 - h
if l.compact {
tslocum commented 1 year ago
Owner

To reduce duplication, please update the implementation to simply skip l.showSecondaryText when compact mode is enabled and there is no secondary text in the list. For instance, the condition:

} else if l.showSecondaryText {

could be updated to take l.compact into account. This should allow this feature to be added without adding any additional special cases outside of skipping l.showSecondaryText

To reduce duplication, please update the implementation to simply skip l.showSecondaryText when compact mode is enabled and there is no secondary text in the list. For instance, the condition: ` } else if l.showSecondaryText {` could be updated to take l.compact into account. This should allow this feature to be added without adding any additional special cases outside of skipping l.showSecondaryText
caninodev changed title from add compact flag to WIP: add compact flag 1 year ago
Poster

To tell you the truth, I am an aspiring dev. So not sure how to go about doing a proper PR. When I created the PR, I used on the web editing feature on your repo to make it. But to make these changes, I don't know how to use the web interface to edit. Can you walk me through this?

To tell you the truth, I am an aspiring dev. So not sure how to go about doing a proper PR. When I created the PR, I used on the web editing feature on your repo to make it. But to make these changes, I don't know how to use the web interface to edit. Can you walk me through this?
Poster

Additionally, I am working on resolving a bug to make this PR feature-complete: namely, that if selectedAlwaysCentered flag is false and compact flag is true, it doesn't work as it should. It behaves as if selectedAlwaysCentered is true.

Additionally, I am working on resolving a bug to make this PR feature-complete: namely, that if `selectedAlwaysCentered` flag is false and `compact` flag is true, it doesn't work as it should. It behaves as if `selectedAlwaysCentered` is true.

Reviewers

tslocum requested changes 1 year ago
This pull request has changes conflicting with the target branch.
list.go
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.