-
Notifications
You must be signed in to change notification settings - Fork 266
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
refactor(table): use lipgloss table for rendering #617
base: master
Are you sure you want to change the base?
Conversation
I think this is done... There are some outstanding PRs on lipgloss with bugs found during the process. You can test it out interactively in this Bubble Tea PR. Do not commit the changes to PRs on lipgloss Some great next steps would be to test these changes against existing table issues to see if they persist now that we've delegated the rendering to lipgloss Probably still needs a bit of a tidy too to make sure I haven't left a whole bunch of |
Okay, this is nearly ready for review. I did find another bug with text alignment that should be addressed before merging (on the There are also some Edit: the alignment issue is only happening with bubbles, not with my lipgloss branch or master (lipgloss)... Weird thing is the truncation is all happening on the lipgloss side, so still need to figure that out 🤔 |
Right truncation from bubbles table (margins)The issue above happens when the cell styles have margins. When you remove margins from the styling, it doesn't happen anymore. This shows that it's likely that the bubbles table is passing these styles along to the lipgloss table, but the rendered result is being truncated on the right rather than taking the alignment into account. Weird truncation behaviour when table width is fixed t.Run("right-aligned text with fixed width table", func(t *testing.T) {
s := DefaultStyles()
s.Selected = s.Selected.Align(lipgloss.Right)
s.Cell = s.Cell.Align(lipgloss.Right)
biscuits := New(
WithColumns([]Column{
{Title: "Name", Width: 25},
{Title: "Country of Origin", Width: 16},
{Title: "Dunk-able", Width: 12},
}),
WithRows([]Row{
{"Chocolate Digestives", "UK", "Yes"},
{"Tim Tams", "Australia", "No"},
{"Hobnobs", "UK", "Yes"},
}),
WithStyles(s),
)
biscuits.SetWidth(30)
t.Logf("\n%s", ansi.Strip(biscuits.View()))
}) In this case, the |
I'm able to reproduce the row truncation issue in lipgloss' table when I set a margin, so will work on fixing that outside of this PR, then hopefully this should be good to go |
Still very wip - just opening a draft PR here to document the process. This change should fix a lot of open issues with the table bubble
TODO
This change needs the
Height
fix on the Lip Gloss table to work properly. (charmbracelet/lipgloss@a5492bc) need to make a new release that includes the fix before merging this.fix YOffset
remove
proposal.org
from this PRre-review API changes