WIP: add compact flag #83

Draft
caninodev wants to merge 1 commits from caninodev/cview:compact_list into master
First-time contributor

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 2021-12-18 19:40:15 +00:00
28827c1146 add compact flag
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.
tslocum requested changes 2021-12-18 23:10:09 +00:00
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.
@ -194,6 +194,10 @@ type List struct {
// 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
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.
Author
First-time contributor

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?
Owner

The prior is what I meant. Thanks!

The prior is what I meant. Thanks!
@ -521,1 +526,4 @@
// 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) {
Owner

Please rename as SetCompact

Please rename as `SetCompact`
@ -800,3 +814,3 @@
} else {
if l.currentItem-l.itemOffset >= h {
l.itemOffset = l.currentItem + 1 - h
if l.compact {
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 2021-12-21 02:30:34 +00:00
Author
First-time contributor

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?
Author
First-time contributor

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.
This pull request has changes conflicting with the target branch.
  • list.go
You can also view command line instructions.

Step 1:

From your project repository, check out a new branch and test the changes.
git checkout -b caninodev-compact_list master
git pull compact_list

Step 2:

Merge the changes and update on Forgejo.
git checkout master
git merge --no-ff caninodev-compact_list
git push origin master
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: tslocum/cview#83
No description provided.