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(table): do not shrink table with offset #373

Merged
merged 3 commits into from
Oct 17, 2024

Conversation

bashbunni
Copy link
Member

We should fix this before merging. This failing test is to provide context on this issue and how to reproduce it. I believe in this case we should honour the height rather than the offset since we are more likely to see dependents on the height.

@bashbunni bashbunni marked this pull request as ready for review September 24, 2024 16:27
@bashbunni
Copy link
Member Author

Fixed the issue in the second commit :)

@bashbunni bashbunni changed the title test(table): test to show table shrinking with offset fix(table): do not shrink table with offset Sep 24, 2024
@bashbunni bashbunni added the bug Something isn't working label Sep 24, 2024
@meowgorithm
Copy link
Member

Hey Bash! So what's the bug you're solving here exactly?

@bashbunni
Copy link
Member Author

Hey, sorry I thought I has included screenshots or something. When you get to a certain offset, it would shrink the table instead of just moving the cursor, causing the height of the table to get smaller than the height specified by the user. If you run the test on master vs this branch you'll be able to see what I mean. Sorry I'll include images/recordings next time :)

@meowgorithm
Copy link
Member

Wow, what a bug! Anyway, the fix looks good so far. To state the issue simply:

When the table is rendered with an offset the height of the table doesn't always match the actual height that was set.

Does that sound correct?

Here is some code to reproduce the issue:
https://gist.github.com/meowgorithm/99a20cd66739a3d0550213c06da0f9a1

Let's make sure we test this on some real-world examples, including with the Table Bubble.

@bashbunni
Copy link
Member Author

bashbunni commented Oct 17, 2024

@meowgorithm yeah that's right! The table bubble doesn't currently use lipgloss table, that's how I discovered this bug was through porting the table bubble to use lipgloss' table renderer. As soon as there was the interactivity element, it broke.

This is the table bubble wip - charmbracelet/bubbles#617 that you can use with this branch. I've actually got a working demo too on a drafted Bubble Tea PR that you can check out. Included instructions there for what to change in your go.work file to test locally.

charmbracelet/bubbletea#1168

@meowgorithm
Copy link
Member

Aha! That's right, table in Bubbles is totally its own thing. Okay cool, I'll test a bit more but generally this looks good.

Copy link
Member

@meowgorithm meowgorithm left a comment

Choose a reason for hiding this comment

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

Alright, LGTM. Such a good fix!!

@bashbunni bashbunni merged commit a5618cb into master Oct 17, 2024
10 checks passed
@bashbunni bashbunni deleted the fix-table-shrink-with-offset branch October 17, 2024 16:32
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
Development

Successfully merging this pull request may close these issues.

2 participants