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

Columns seem to ignore lipgloss padding #130

Open
misterikkit opened this issue Jan 27, 2023 · 7 comments
Open

Columns seem to ignore lipgloss padding #130

misterikkit opened this issue Jan 27, 2023 · 7 comments
Labels
bug Something isn't working

Comments

@misterikkit
Copy link

I want to pad my cell data with a space on both the left and the right. This is because in many terminals, double-clicking on the text of a cell will select the contents plus the column separator. The common behavior is to delimit only on whitespace. What I want is to quickly copy a cell's contents into my OS clipboard for pasting into other tools.

I tried this by adding a column style that uses .Padding(0, 1) as well as manually adding spaces to the row data, but either way the table seems to trim space before rendering. Any hints on how I should be doing this?

I skimmed through the code and did not find any explicit logic to remove leading or trailing space.

Note: For overflow, I don't care if the ellipsis touches the column separator, because a truncated value is not worth copying into a clipboard.

I am using Version: v0.15.0

Sample Program:

package main

import (
	"fmt"

	"github.com/charmbracelet/lipgloss"
	"github.com/evertras/bubble-table/table"
)

func main() {

	cols := []table.Column{
		table.NewColumn("a", "A", 10).
			WithStyle(
				lipgloss.NewStyle().Padding(0, 1),
			),
	}

	rows := []table.Row{
		table.NewRow(table.RowData{"a": "hello"}),
		table.NewRow(table.RowData{"a": "very long text"}),
	}

	t := table.New(cols).WithRows(rows)
	fmt.Println(t.View())
}

Expected Output:

┏━━━━━━━━━━┓
┃        A ┃
┣━━━━━━━━━━┫
┃    hello ┃
┃ very lo… ┃
┗━━━━━━━━━━┛

Actual Output:

┏━━━━━━━━━━┓
┃         A┃
┣━━━━━━━━━━┫
┃     hello┃
┃very long…┃
┗━━━━━━━━━━┛
@misterikkit
Copy link
Author

Interesting result: I can get closer to the format I want with table.NewStyledCell

package main

import (
	"fmt"

	"github.com/charmbracelet/lipgloss"
	"github.com/evertras/bubble-table/table"
)

func main() {

	cols := []table.Column{
		table.NewColumn("a", "A", 10),
	}

	paddedCell := lipgloss.NewStyle().Padding(0, 1) // for our own safety
	rows := []table.Row{
		table.NewRow(table.RowData{"a": table.NewStyledCell("hello", paddedCell)}),
		table.NewRow(table.RowData{"a": table.NewStyledCell("very long text", paddedCell)}),
	}

	t := table.New(cols).WithRows(rows)
	fmt.Println(t.View())
}

Output

┏━━━━━━━━━━┓
┃         A┃
┣━━━━━━━━━━┫
┃    hello ┃
┃     very ┃
┃    long… ┃
┗━━━━━━━━━━┛

@Evertras
Copy link
Owner

Apologies for the late reply on this, it slipped through my email and I haven't checked here for a little bit. I think the StyledCell approach is probably going to be the way to go here, but it does feel a bit rough that your original attempt didn't work. I'm going to leave this open for now, but I'm not entirely sure what the obvious fix will be and there is a workaround. 🤔

@Evertras Evertras added the bug Something isn't working label Feb 13, 2023
@prgres
Copy link
Contributor

prgres commented Nov 27, 2023

What I found so far is this line. cellStyle is built by coping rowStyle  and inherit a column.style

func (m Model) renderRowColumnData(row Row, column Column, rowStyle lipgloss.Style, borderStyle lipgloss.Style) string {
	cellStyle := rowStyle.Copy().Inherit(column.style).Inherit(m.baseStyle)

In its implementation in the lipgloss package, we can see that margins and padding properties are skipped


func (s Style) Inherit(i Style) Style {
	s.init()

	for k, v := range i.rules {
		switch k {
		case marginTopKey, marginRightKey, marginBottomKey, marginLeftKey:
			// Margins are not inherited
			continue
		case paddingTopKey, paddingRightKey, paddingBottomKey, paddingLeftKey:
			// Padding is not inherited
			continue
		case backgroundKey:
			s.rules[k] = v

			// The margins also inherit the background color
			if !s.isSet(marginBackgroundKey) && !i.isSet(marginBackgroundKey) {
				s.rules[marginBackgroundKey] = v
			}
		}

		if _, exists := s.rules[k]; exists {
			continue
		}
		s.rules[k] = v
	}
	return s
}

With that knowledge, I created a column with left padding

	columns := []table.Column{
		table.NewColumn(columnKeyName, "Name", 10).WithStyle(
			lipgloss.NewStyle().
				Foreground(lipgloss.Color("#88f")),
		),
		table.NewColumn(columnKeyCountry, "Country", 20).WithStyle(
			lipgloss.NewStyle().
				Foreground(lipgloss.Color("#f88")).
				PaddingLeft(5),
		),
		table.NewColumn(columnKeyCurrency, "Currency", 10),

and change the renderRowColumnData a little bit

func (m Model) renderRowColumnData(row Row, column Column, rowStyle lipgloss.Style, borderStyle lipgloss.Style) string {
	cellStyle := rowStyle.Copy().Inherit(column.style).Inherit(m.baseStyle)

	_, _, _, colPadLeft := column.style.GetPadding()
	if colPadLeft > 0 {
		cellStyle = cellStyle.PaddingLeft(colPadLeft)
	}

Which results in the desired state:
image


What we can do is:

  • modify the Inherit method or create a custom one overriding all values from the incoming style - this will be impossible to make in a finite period since contributions to charm are hella slow (or they do not like me dunno)
  • append that logic into renderRowColumnData (for paddings and margins)

I have some free time now, I can go with the second solution. cc. @Evertras

@prgres
Copy link
Contributor

prgres commented Nov 27, 2023

Okay, it may be tricky because GetPadding and GetMargins return values or 0 if a property is not set. Creating a simple if statement as I did may result in a situation when a rowStyle sets padding to >0 value and the user wants to have a column with padding ==0 and it will not be overridden.

Without access lipgloss.Style.rules it may be hard to implement simply. We can cover the override margins/paddings with a next column flag but I do not if it is a best approach here

@prgres
Copy link
Contributor

prgres commented Nov 27, 2023

charmbracelet/lipgloss#71 😆

@prgres
Copy link
Contributor

prgres commented Nov 27, 2023

It came out that we cannot set paddings and margins in base style because it breaks the layout

PaddingRight(10)
image

MarginRight()
image

So my objects are invalid now, I am going to create a PR with that

@prgres
Copy link
Contributor

prgres commented Nov 27, 2023

PR: #160

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

No branches or pull requests

3 participants