-
Notifications
You must be signed in to change notification settings - Fork 260
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(viewport): fix height calculation method #578
base: master
Are you sure you want to change the base?
Conversation
@caarlos0 |
Hey @kyu08 thanks so much for this contribution. It looks good and all the tests are passing. Thank you as well for including a test with the changes, makes this PR super easy to review. I just tried running some bubble tea examples that use Let me know if you're able to reproduce that issue :) |
0fdf18e
to
5e1cef4
Compare
Thanks for your kind words! I am also grateful to maintainers like you for making this project available. Also, thank you for finding the bug.
I fixed that issue ( eaff7e7 ) and the another issue that percentage counter is not working properly.( 5e1cef4 ) I noticed that we should not use Could you re-review them please? (If my changes are okey, I can squash commits if needed.) |
`GoToBottom()` should be called when the offset exceeds the number of actual lines, not the number of content lines.
|
@bashbunni |
Hey @kyu08 will do when I'm able to give some attention to bubbles in the next little bit. Usually I alternate focuses on a weekly basis, so will let you know when I'm on an open source maintenance week :D |
That said, I'm very eager to include this in v0.20.0, so will definitely be taking a look when I can |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey I just refactored to make the names more clear + wrap words rather than cutting them. I also added a test case to reflect that.
I was getting an out of range error on this if I sized the terminal window very small, scrolled down to the very bottom, then expanded it as large as possible. Was just an issue with the YOffset exceeding its max. Fixed with a WindowSizeMsg
check in the Update
@bashbunni The tests failing seems like fixed by this: diff --git a/viewport/viewport.go b/viewport/viewport.go
index 05e93ae..332134b 100644
--- a/viewport/viewport.go
+++ b/viewport/viewport.go
@@ -423,7 +423,7 @@ func wrap(lines []string, width int) []string {
// wrap lines (handles lines that could not be word wrapped)
wrap := ansi.Hardwrap(wrapWords, width, true)
// split string by new lines
- wrapLines := strings.Split(strings.TrimSpace(wrap), "\n")
+ wrapLines := strings.Split(wrap, "\n")
out = append(out, wrapLines...)
}
diff --git a/viewport/viewport_test.go b/viewport/viewport_test.go
index 0dc7974..3863255 100644
--- a/viewport/viewport_test.go
+++ b/viewport/viewport_test.go
@@ -31,6 +31,11 @@ func TestWrap(t *testing.T) {
width: 12,
want: []string{"hello bob, I", "like yogurt", "in the", "mornings."},
},
+ "whitespace of head of line is preserved": {
+ lines: []string{" aaa", "bbb", "ccc"},
+ width: 5,
+ want: []string{" aaa", "bbb", "ccc"},
+ },
} How about deleting |
Resolves #479
The issue I experienced
I found that example app of viewport can't render properly if content has very long line.
If the content using in the example's was like below in the toggle, even if the content was scrolled to the bottom, the example can't render all contents. The content of last line(
This is the last line.
) is not rendered.Content for reproduce
What this PR fixes
This PR fixes the problem to fix height calculation method.
Without this change a very long line that exceeds width is not considered. New method considers it.