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

Table manual Height() logic improvement #406

Open
Apsu opened this issue Oct 19, 2024 · 4 comments
Open

Table manual Height() logic improvement #406

Apsu opened this issue Oct 19, 2024 · 4 comments
Labels
enhancement New feature or request question Further information is requested

Comments

@Apsu
Copy link

Apsu commented Oct 19, 2024

Is your feature request related to a problem? Please describe.
I would like to be able to set a height that a lipgloss.Table will render at inside its "viewport" (visible area for rows, after border/header/padding/etc), bubbles.Table style. This would allow for easier full-screen TUIs where the table always fills the terminal space, and resizes on SIGWINCH/tea.WindowSizeMsg, regardless of the content in the table.

I would think that setting a manual Height() should attempt to do this, given there's a useManualHeight flag tracked when you call Height(), but there's a clamp check here that limits the rendering when there's less rows than the availableLines (essentially a viewport calculation).

Describe the solution you'd like
Without setting Height(), and thus without the useManualHeight flag, the table height sizes to match the number of rows always. With setting Height(), the clamp still only matches the number of rows, but now contracts when the rows > the manual height. I believe the desired behavior would be:

  • No height set
    • Always size table to rows
    • Possibly clamp to terminal height
      • Might still be use-cases people want of putting table inside scrollable viewport so maybe even this shouldn't be assumed, and current implementation is fine
  • Height set
    • Always size table to height
    • Less rows than height, pad remaining space
    • More rows than height, only display what fits

Describe alternatives you've considered
bubbles.Table has roughly this behavior now, and I was using it at first, as I also want cursor nav/select, but it doesn't auto-size columns. I started doing that myself, but then decided to implement a cursor on lipgloss.Table, and discovered the height clamping above. I implemented my own row padding and viewport calculations since those fields are internal to lipgloss.Table, but it would be nicer and I think a more expected behavior to just set Height() and have the table do the padding/layout to maintain that height for users, since the internal state can do it better than an external padding.

Additional context
Example of padding approach I'm using:

func (t *Table) Rows(filledRows TableRows) {
	// Store filled rows for refills
	t.rows = filledRows
	t.length = len(filledRows)

	// This allows table to maintain desired viewport when not full
	var displayRows TableRows
	// Pad rows to viewport if less filled than offset viewport
	if t.length < t.offset+t.viewport {
		displayRows = append(filledRows, make(TableRows, t.offset+t.viewport-t.length)...)
        // Or trim to offset viewport
	} else {
		displayRows = filledRows[:t.offset+t.viewport]
	}

	// Convert for underlying lipgloss table
	converted := make([][]string, len(displayRows))
	for i, row := range displayRows {
		converted[i] = []string(row)
	}

	// Replace lipgloss table data
	t.table.Data(lgt.NewStringData(converted...))
}
@Apsu Apsu added the enhancement New feature or request label Oct 19, 2024
@bashbunni
Copy link
Member

bashbunni commented Oct 21, 2024

Hey @Apsu I'd love to see an example of what you're thinking for a fixed height table with less rows than the height. I can see where you're coming from especially if you're using it in a layout. I'll need to look at how we historically compare Height and MaxHeight functions in lipgloss.

Maybe
Height -> the table should always be this height (padded if needed; padding)
MaxHeight -> the table will never exceed this height (but can be smaller given the number of rows; no padding)

Next we would also need to clarify what that table would look like depending on the border settings.

A) (pretend outer borders aren't broken........)

`
┌──────────┬──────────────┬───────────┐
│ LANGUAGE │    FORMAL    │ INFORMAL  │
├──────────┼──────────────┼───────────┤
│ Chinese  │ Nǐn hǎo      │ Nǐ hǎo    │
│ French   │ Bonjour      │ Salut     │
│ Japanese │ こんにちは   │ やあ      │
│ Russian  │ Zdravstvuyte │ Privet    │
│ Spanish  │ Hola         │ ¿Qué tal? │






└──────────┴──────────────┴───────────┘
`

Then would it pad the last row of cells if there is an internal border set? or create empty rows?

@bashbunni
Copy link
Member

fwiw the Height function for lipgloss table is still a bit bugged. I noticed recently that we don't account for whitespace when calculating the height, so there is still a pending fix there too #400

@bashbunni
Copy link
Member

Note: should look at how we're doing this in huh. It does make sense to have a consistent height so it doesn't break styling when included in layouts. Maybe we pad after the table to the set height OR have space included within the table as mentioned above... Will discuss with the team 💭

@bashbunni bashbunni added the question Further information is requested label Dec 2, 2024
@Apsu
Copy link
Author

Apsu commented Dec 6, 2024

Heyo! I know I added this then kind of abandoned it, got very busy lol. Glad to see you're still thinking about the best approach here.

To your first question, I was using empty rows just to force the behavior to give the visual result I wanted, but if I had access to the internal fields in the library I'm sure it'd be easy to render the outer borders to the desired height without managing blank row entries. Internal borders though I.... feel like might be wanted for some use-cases and not for others? Maybe should be a toggle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants