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: render table with lipgloss #262

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Conversation

bashbunni
Copy link
Member

@bashbunni bashbunni commented Sep 27, 2023

This is still a wip, but making a draft PR so it's easier to get feedback and help debugging along the way.

It seems to spit out the right table most of the time, but I'd still like to...

  • remove StyleWriter dep in table
  • fix failing test (or understand why it's failing) ansi/TestRendererIssues/46_2
  • apply styling to cells and rows
  • add margin support

@bashbunni
Copy link
Member Author

bashbunni commented Sep 28, 2023

I still don't know why the one test is failing, but I've added a basic padded cell style and support text styling for the cells. I wonder if using renderText for styling the text is the best call. It might end up making more sense to have the styling separate from writing so it's easier to amend styles throughout the program.

@bashbunni
Copy link
Member Author

bashbunni commented Sep 29, 2023

Going to work on margin support in another PR - the scope of this one is just table rendering

@bashbunni
Copy link
Member Author

I believe this is using the Lip Gloss table in its pre-release state, so will need to tidy this up a bit before she's ready 2 rock

@bashbunni bashbunni linked an issue Apr 4, 2024 that may be closed by this pull request
@bashbunni bashbunni removed a link to an issue Apr 4, 2024
@bashbunni bashbunni linked an issue Apr 4, 2024 that may be closed by this pull request
renderText(ctx.table.styleWriter, ctx.options.ColorProfile, style, rules.Prefix)
ctx.table.writer = tablewriter.NewWriter(ctx.table.styleWriter)
renderText(w, ctx.options.ColorProfile, style, rules.Prefix)
ctx.table.lipgloss = table.New().StyleFunc(func(row, col int) lipgloss.Style { return cellStyle })
Copy link

Choose a reason for hiding this comment

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

I've tested the PR on some of the large tables that I'd like to display with it and it already looks a lot better! However, I noticed that the width is not properly set, this does the trick:

Suggested change
ctx.table.lipgloss = table.New().StyleFunc(func(row, col int) lipgloss.Style { return cellStyle })
ctx.table.lipgloss = table.New().
StyleFunc(func(row, col int) lipgloss.Style { return cellStyle }).
Width(int(ctx.blockStack.Width(ctx)))

The downside is that it causes columns which contain rows with long text1 to be truncated or even omitted entirely. I'd prefer if it either wraps text within the cell or the table is printed in full width (which isn't really possible because it seems like glamour cuts off the lines, trying to wrap them?). But this is probably a feature request towards lipgloss and not glamour.

Footnotes

  1. If there's 100 empty rows and one with 100 characters the column is more likely to get truncated as explained here.

@gamebox
Copy link

gamebox commented Apr 18, 2024

Maybe before it's considered perfect you could release this as a TermRendererOption (WithLipglossTableRenderer)? And then when you are satisfied, you can deprecate that option and noop it?

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.

refactor: replace TableWriter with lipgloss table
3 participants