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

fix(group): do not render newline before empty help #200

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ardnew
Copy link
Contributor

@ardnew ardnew commented Apr 29, 2024

This is the same change as PR #117, which was closed due to my incompetence with git. This is now baselined against the latest main (f922e26).

Note that since #117, upstream charmbracelet/huh changed the separator "gap" between group content view and keybindings/errors view from using the current theme (g.theme.FieldSeparator.String()) to always using the literal line feed ('\n'). Assuming that was intentional, I've maintained that behavior in this PR.

@ardnew ardnew requested a review from maaslalani as a code owner April 29, 2024 17:31
group.go Outdated
}
}
if foot.Len() > 0 && hasNonSpace(foot.String()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could also simply check:

Suggested change
if foot.Len() > 0 && hasNonSpace(foot.String()) {
if strings.TrimSpace(foot.String()) != "" {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Errr, right, of course. Thanks 😇

hasNonSpace seems a little silly at second glance. Will remove its definition entirely

@maaslalani
Copy link
Contributor

Do you mind posting an example of a before and after screenshot of which scenario view this fixes, thank you so much!

@ardnew ardnew force-pushed the fix/trim-empty-help branch from 53f853d to dc7912f Compare May 7, 2024 19:12
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