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: wrapping and height calculations #485

Draft
wants to merge 22 commits into
base: main
Choose a base branch
from
Draft

Conversation

bashbunni
Copy link
Member

@bashbunni bashbunni commented Nov 26, 2024

Changes

Content used to cut off if the height of the window was smaller than the content height.
Wrapped content broke formatting and caused content to get cut off.
The offset calculations were off and wouldn't scroll to show hidden content.
There was an off by one error for content that exceeded the window height (would always hide the title of the first field).

Other considerations

Some fieds (confirm, select, input) have the option to be shown as inline. When shown as inline, the styles do not wrap (intentional)

Buttons will stack vertically if they don't fit horizontally. Button widths will also match (do we want that by default or only when stacked?)

Fixes

closes #398
closes #429

Maybe others (need to confirm)

Here's what this PR looks like:
https://github.com/user-attachments/assets/b3588782-33d4-4e11-8105-2f6b7335df6a

Previous behaviour:
https://github.com/user-attachments/assets/0e77808e-5895-45d4-b075-f299babecf2b

A few enhancements I'd still like to see: huh does a lot of redraws that cause rendering issues when not in alt screen. You come across that behaviour when resizing the window a lot while running an example (e.g. burger)

  • feat: set group with from size msg
  • feat: handle WindowSizeMsg + base height on rendered content
  • feat: render prefix before newlines
  • feat: update viewport on resize, cursor != height so no scroll...
  • fix: always update viewport height
  • fix: add option prefix
  • fix(multiselect): width calculation
  • fix: calculate offset from previous field heights
  • feat: add calculate wrapping helper
  • fix(select): wrap select field
  • feat: make group set field widths accounting for styles
  • feat: set width
  • feat: stack buttons in narrow window
  • feat: add Theme function to Fields
  • feat: calculate width based on field theme
  • fix(group): fix height calculation
  • feat: remove FullWidth helper (not needed)
  • fix(group): fix off by one at top for overflowed content

@@ -521,7 +524,7 @@ func (f *Form) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
}
f.selector.Range(func(_ int, group *Group) bool {
if group.fullHeight() > msg.Height {
group.WithHeight(msg.Height)
group.WithHeight(msg.Height - 1)
Copy link
Member Author

Choose a reason for hiding this comment

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

this accounts for the help menu at the bottom of the window taking up one row. Can add a const for this 1 for legibility if you'd prefer

Copy link
Member

Choose a reason for hiding this comment

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

i think just a comment would be ok enough 🤔

eg:

Suggested change
group.WithHeight(msg.Height - 1)
group.WithHeight(msg.Height - 1) // subtracts help height

@@ -285,25 +288,26 @@ func (g *Group) fullHeight() int {
height := g.selector.Total()
g.selector.Range(func(_ int, field Field) bool {
height += lipgloss.Height(field.View())
height += lipgloss.Height(gap)
Copy link
Member Author

Choose a reason for hiding this comment

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

without this line, content that fits in the current window size will truncate its height slightly

what it looks like without this line:
https://github.com/user-attachments/assets/7a3a0d33-2524-49ca-b334-02ba072af10d

@bashbunni bashbunni marked this pull request as ready for review November 27, 2024 22:30
@bashbunni
Copy link
Member Author

In case we don't want to merge the stacked buttons here, I have another version of this branch where I removed those features. https://github.com/charmbracelet/huh/tree/horizontal-btns

@bashbunni
Copy link
Member Author

bashbunni commented Dec 2, 2024

rip, looks like these changes might be breaking grid layout...

image

In the image above we're missing options 4-6. This is happening at all widths. Absolutely tragic 😭

Build info:
macOS, kitty, no multiplexer, just running the layouts/grid example

  • add tests with outputs from examples

@bashbunni bashbunni added the bug Something isn't working label Dec 2, 2024
@bashbunni bashbunni marked this pull request as draft December 2, 2024 16:40
Comment on lines +526 to +531
theme := s.theme
if theme == nil {
theme = ThemeCharm()
}
return theme
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
theme := s.theme
if theme == nil {
theme = ThemeCharm()
}
return theme
}
if s.theme != nil {
return s.theme
}
return ThemeCharm()
}

feels a bit better I think 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

so true, I can update this for all fields

@@ -268,7 +270,8 @@ func (g *Group) Update(msg tea.Msg) (tea.Model, tea.Cmd) {

switch msg := msg.(type) {
case tea.WindowSizeMsg:
g.WithHeight(max(g.height, min(g.fullHeight(), msg.Height-1)))
g.WithHeight(min(g.fullHeight(), msg.Height-1))
Copy link
Member

Choose a reason for hiding this comment

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

what is this -1 here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants