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

fix: column style respects paddings and margins #160

Closed
wants to merge 1 commit into from

Conversation

prgres
Copy link
Contributor

@prgres prgres commented Nov 27, 2023

Address #130

A little context (copied and pasted from the issue):

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)

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


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

PaddingRight(10)
image

MarginRight()
image

@Evertras
Copy link
Owner

Taking a moment to play with this, want to see if there's some potential way to break it...

@Evertras
Copy link
Owner

Unfortunately, this doesn't seem to work as expected for me, or for the #130 code sample. Using your branch I updated the feature example column:

table.NewColumn(columnKeyDescription, "Description", 30).WithStyle(
  lipgloss.NewStyle().PaddingLeft(3),
),

The result is unchanged:

image

I would have expected the padding to be applied to the Description column. Additionally, running the sample given in #130 doesn't give the expected output.

Oddly, using your given example, I still don't see the padding change. I do see the foreground color update, but the columns are still strictly left aligned. I feel like something might be odd here, but I'm not sure what. I confirmed I have the actual code changes in my local copy with your branch checked out.

It may also be helpful to create a test with the code in #130 to see if we can use that as a specific target.

@prgres
Copy link
Contributor Author

prgres commented Dec 1, 2023

Well, that's weird. I will hop on the debug in a spare time

@Evertras
Copy link
Owner

Are you still taking a look at this? If not I'd like to close for cleaning things up, but we can always reopen later.

@Evertras
Copy link
Owner

Closing this for now for tidiness, but we can reopen again later if you dig back in.

@Evertras Evertras closed this Mar 12, 2024
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.

None yet

2 participants