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

feat(table): improve sizing and behavior: wrap by default, overflow optionally #479

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

andreynering
Copy link
Member

@andreynering andreynering commented Feb 24, 2025

Hey everybody,

The whole work took me multiple days to get right, with a lot of testing and back-and-fourth.

I think it is very close to the ideal now. We may want to do small adjustments, and in this regard I ask for your opinion.

Highlights:

  • The whole resizing logic was moved into a separate file, using multiple functions. Before, it was all done inside the Table.String() function.
  • Wrapping will now happen by default.
  • If wrapping is disabled with t.Wrap(false), the behavior of adding a trailing is now fixed. Before, it was just cutting the content most of the time.

Note

There are many issues related to table rendering. I think this PR might fix all of them. I vote to close all these issues to clean the list up, and if any user still face any minor issue, they can still comment or open new issues to let us know.


Screenshots

Before Screenshot 2025-02-24 at 17 37 36
After Screenshot 2025-02-24 at 17 33 48 Screenshot 2025-02-24 at 17 35 06

@andreynering andreynering force-pushed the table-resize branch 2 times, most recently from 3b5295e to b1e1e9f Compare February 24, 2025 18:24
@caarlos0
Copy link
Member

can you put some screenshots of before/after?

@andreynering
Copy link
Member Author

@caarlos0 I just added some screenshot to the PR body. They are from the ExampleWrap function, so you can check them locally if you want.

@andreynering
Copy link
Member Author

Also, thanks @bashbunni for all the previous work on this! I end up refactoring most of the code, but many of the new tests and examples were taken from her branches.

@testinfected
Copy link

Amazing! Many thanks for that @andreynering

Copy link
Member

@bashbunni bashbunni left a comment

Choose a reason for hiding this comment

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

Lip Gloss

Examples

✅ Same as master - RTL languages break on Apple's Terminal. Fine in iterm2 and kitty. Probably more to do with ansi package and terminal emulator capabilities.
image

✅ Same as master - when you resize the terminal window smaller than the table, then change it back to an acceptable size it doesn't redraw quite right
image

✅ FIXED [Not on master - runtime error when running pokemon table example]
image

Tests

All good! ✅

Glamour

✅ Artichoke Example (based on v0.7.0)

This is the only example with a table.

Before: (Glamour @ v0.7.0 - pre Lip Gloss table)
image

After: (Glamour @ master using this branch for Lip Gloss table)
image

I think this change in style looks good and that the table sizing makes more sense in the "after" image.

Tests

Some tests fail with these changes. Haven't looked deeper on this, but may be worth addressing. Note that these could be due to adaptations we need to make to lipgloss table's usage in glamour's code base, not necessarily caused by the changes in this PR.

  • TestRendererIssues/60 - right padding or margin distribution is off given the column sizes + contents.

image

  • TestRendererIssues/46_2 - Unexpected spacing between rows

image

  • TestRendererIssues/44 - Unexpected spacing between rows
    image

As mentioned above, there are other tests that are failing and likely need to be updated to work with these changes. That said, all tests pass on Glamour's master branch, so that may indicate that this branch introduces some breaking changes. Again, needs a bit more digging to figure out why these tests are failing.

@andreynering
Copy link
Member Author

@bashbunni Thanks for your detailed review. I just opened charmbracelet/glamour#394 to make the tests pass on Glamour.

@andreynering
Copy link
Member Author

@bashbunni I just fixed the bug causing a panic on the Pokemon example.

@bashbunni
Copy link
Member

Note: this feature should not be merged until charmbracelet/x#350 is merged and released as the new cellbuf.Wrap allows us to have clean wrapping of pre-defined styles in tables. We need to support that for Glamour

Comment on lines -7 to +9
go 1.18
go 1.23.0

toolchain go1.23.1
Copy link
Member

Choose a reason for hiding this comment

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

I would leave this at 1.18

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment