WIP: add compact flag #83

caninodev · 2021-12-18T19:40:15Z

caninodev commented

2021-12-18 19:40:15 +00:00

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

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.

list.go

													
				@ -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

tslocum commented

2021-12-18 23:09:26 +00:00

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.

caninodev commented

2021-12-21 03:11:21 +00:00

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

2021-12-21 20:54:36 +00:00

The prior is what I meant. Thanks!

list.go

													
				@ -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) {

tslocum commented

2021-12-18 23:02:06 +00:00

Please rename as SetCompact

Please rename as `SetCompact`

list.go

													
				@ -800,3 +814,3 @@

					} else {

						if l.currentItem-l.itemOffset >= h {

							l.itemOffset = l.currentItem + 1 - h

							if l.compact {

tslocum commented

2021-12-18 23:05:31 +00:00

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

caninodev commented

2021-12-22 19:14:37 +00:00

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?

caninodev commented

2021-12-23 15:05:46 +00:00

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.

This pull request has changes conflicting with the target branch.