Skip to content

Conversation

@andreynering
Copy link
Member

@andreynering andreynering commented Mar 5, 2025

This PR is basically to just update the tests to make them pass, as there are some small differences with the new table work.

Before merging:

@bashbunni
Copy link
Contributor

If I run these in ansi with go test ./... all tests pass, but when I use the -run directive, they fail 🤔 It seems that it's adding a selected row style to the first row by default when I run go test -run TestRendererIssues

image
vs
image

@bashbunni
Copy link
Contributor

Just added a commit that fixes the issue I reported above

@bashbunni
Copy link
Contributor

It looks like we're still adding extra lines between rows. There should also be ellipsis if we're cutting content (see Architectures column). I think we updated the test data though, so tests are passing

Test outputs

TestRendererIssues/44

Expected:
image
Got:
image

@andreynering
Copy link
Member Author

Thank you @bashbunni!

I disabled content wrapping, which ensures we're having the same behavior as before, but with the fix of showing the ellipsis now as opposed to just cutting the content.

@bashbunni
Copy link
Contributor

In v0.7.0 content wrapped by default. I just added a commit to wrap the table content by default and add some default margins which should get overridden by custom styles if some are given (will double check)

However, it seems the ANSI escape sequences aren't closing properly with wrapped content. I'm going to double check with a test on the Lip Gloss side to see if this is an issue with the new wrapping behaviour or not

image

PS: I updated some golden files based on outputs from v0.7.0 when we were using the tablewriter package https://github.com/olekukonko/tablewriter. We can update the golden files once we sort out this style wrapping stuff

Side note: I'm not sure if glamour is doing anything with the Conceal directive that you can set in styles. One idea I had for rendering links in the table is just to conceal the URL in the table and only display LinkText, then add all links underneath the table so you can actually click them.

@bashbunni
Copy link
Contributor

Just checked in Lip Gloss and was able to reproduce this. Added a test TestTableWithLinks that showcases this behaviour
image

@bashbunni
Copy link
Contributor

Using cellbuf.Wrap from charmbracelet/x fixes this issue. Already added that to the Lip Gloss PR that will fix this stuff.

The only thing left is to make sure that headers show "…" if the content needs to truncate

image

@andreynering andreynering marked this pull request as ready for review March 12, 2025 13:13
@andreynering
Copy link
Member Author

After update:

Screenshot 2025-03-12 at 10 16 41

@aymanbagabas A potential improvement to x/cellbuf would be to avoid breaking ponctuation characters like . , ; etc. Not sure if we want to block a release just because of this, though.

@aymanbagabas
Copy link
Member

After update:

Screenshot 2025-03-12 at 10 16 41 @aymanbagabas A potential improvement to `x/cellbuf` would be to avoid breaking ponctuation characters like `.` `,` `;` etc. Not sure if we want to block a release just because of this, though.

Could you expand on this?

One issue I see is stripping white space characters at the end and beginning of the wrapped content.

@andreynering
Copy link
Member Author

andreynering commented Mar 12, 2025

@aymanbagabas I meant about the fact that some of the lines are starting with ,, while the whole word + the , char should have been moved to the next line.

I went to write a test case for this on x/cellbuf, but find out that it is actually working as expected for words without any formatting. I suppose that the bug is because each of the words is formatted on the example, but the following , is not. Looks like the end of the formatting is forcing the word to break?

@andreynering andreynering merged commit f43b1ad into master Mar 13, 2025
14 of 22 checks passed
@andreynering andreynering deleted the table-resize branch March 13, 2025 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants