Skip to content

Center log indicator#696

Merged
derailed merged 1 commit intoderailed:masterfrom
ckoehn:fix/center-log-indicator
May 7, 2020
Merged

Center log indicator#696
derailed merged 1 commit intoderailed:masterfrom
ckoehn:fix/center-log-indicator

Conversation

@ckoehn
Copy link
Copy Markdown
Contributor

@ckoehn ckoehn commented May 2, 2020

Previously all options were padded with spaces which caused the indicator to be visually uncentered.

Comment thread internal/view/log_indicator.go Outdated
@@ -107,5 +108,5 @@ func (l *LogIndicator) onOff(b bool) string {
}

func (l *LogIndicator) update(status string) {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ckoehn Thank you for this PR! Good eye!! I think we could refactor the update method as follows:
Does this make sense?

func (l *LogIndicator) update(title string, state bool, pad ...bool) {
	var padding string
	if len(pad) == 0 {
		padding = spacer
	}
	fmt.Fprintf(l, "[::b]%s:%s%s", title, l.onOff(state), padding)
}

Copy link
Copy Markdown
Contributor Author

@ckoehn ckoehn May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't understand why pad is a variadic argument and changed it to a non-variadic one.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that's fine. Thank you Christian!

Previously all options were padded with spaces which caused the
indicator to be visually uncentered.
@derailed derailed merged commit b293395 into derailed:master May 7, 2020
@ckoehn ckoehn deleted the fix/center-log-indicator branch May 7, 2020 13:02
placintaalexandru pushed a commit to placintaalexandru/k9s that referenced this pull request Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants