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: shared indices for first row of data and headers (StyleFunc bug) #377

Merged
merged 6 commits into from
Oct 3, 2024

Conversation

bashbunni
Copy link
Member

@bashbunni bashbunni commented Sep 24, 2024

While migrating the table bubble to use Lip Gloss, I noticed some rendering issues where the cursor was constantly off by one. After fixing the off by one error, I was still having some range issues where the first element was never showing the selected style. Turns out, both the first element in the list and the header are the same index, so whatever style was returned first would be rendered by both the header and first element in the list.

Despite the cursor being in the right position, it was rendering the next element in the list with the declared styles.

Note

This is a bit of a breaking change, though necessary for the first element to ever render correctly when there are also styles set on the headers. This would require table users to update their StyleFunc implementations to use HeaderRow instead of 0.

Before:
image

After:
image
(it's failing intentionally so I could see the outputs)

This PR introduces a test to Lip Gloss that adds unique styles to both the header and first row of data to showcase the issue. When running the test against master, you'd need to comment out the HeaderRow stuff and you'll see that both the first row and header use the same style.

@bashbunni bashbunni added the bug Something isn't working label Sep 24, 2024
@bashbunni bashbunni marked this pull request as draft September 24, 2024 14:52
@bashbunni bashbunni marked this pull request as ready for review September 24, 2024 16:23
@bashbunni
Copy link
Member Author

This is the test that reproduces the behaviour I was seeing in the bubble (in the screenshots above) https://github.com/charmbracelet/lipgloss/blob/fix-stylefunc/table/table_test.go#L1144-L1169

@@ -7,6 +7,10 @@ import (
"github.com/charmbracelet/x/ansi"
)

// HeaderRow denotes the header's row index used when rendering headers. Use
// this value when looking to customize header styles in StyleFunc.
const HeaderRow int = -1
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be exported?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah this is the value they would be using in StyleFunc to style the header instead of 0. I figured better to export a const than have them check for -1

@meowgorithm
Copy link
Member

meowgorithm commented Sep 30, 2024

Thanks, @bashbunni! If it's not too much trouble, can you plop down some source code for a quick program to reproduce this (even if it's based on the tests)? It's helpful to see the before/after interactively to understand what we're lookin' for.

Ah, wait, this is Lip Gloss. Okay, will poke around on this one.

@bashbunni
Copy link
Member Author

@meowgorithm the tests are probably the best place to see the changes!

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.

Nicely done 👌

@bashbunni bashbunni merged commit f8dd507 into master Oct 3, 2024
10 checks passed
@bashbunni bashbunni deleted the fix-stylefunc branch October 3, 2024 17:12
boks1971 added a commit to livekit/livekit-cli that referenced this pull request Nov 1, 2024
lipgloss changed the row numbering in this PR (charmbracelet/lipgloss#377)
boks1971 added a commit to livekit/livekit-cli that referenced this pull request Nov 1, 2024
lipgloss changed the row numbering in this PR (charmbracelet/lipgloss#377)
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.

3 participants