charmbracelet / lipgloss

Style definitions for nice terminal layouts 👄
MIT License
8.22k stars 231 forks source link

feat: move tree to root #342

Closed caarlos0 closed 3 months ago

caarlos0 commented 4 months ago

closes #339 closes #341

meowgorithm commented 4 months ago

If you don't mind, going to look at a few last things today before we merge this one.

bashbunni commented 3 months ago

Hey @meowgorithm any changes on this one?

meowgorithm commented 3 months ago

Let’s get the tree examples back in as well — I'm looking at stuff now.

bashbunni commented 3 months ago

@meowgorithm kk no rush. Just added the examples back

bashbunni commented 3 months ago

Removed JoinVertical from the final rendered element as it was writing unstyled whitespace to the final string. It does that by design to appropriately align elements in the block, but because it doesn't have access to the style, the whitespace can't be rendered. That said, I haven't seen this issue outside of the tree examples, so it's not pressing.

If we did want to fix this we could replace this whitespace rendering with calling Align on the style which can render the whitespace properly or fix this with our compositor in a future version of lipgloss

bashbunni commented 3 months ago

I thought the lipgloss root styling might be broken because if we do

ItemStyleFunc(func(children tree.Children, i int) lipgloss.Style {
    if children.At(i).Value() == "Nyx" {                          
        return itemStyle.Background(lipgloss.Color("#04B575"))    
    }                                                             
    return itemStyle                                              
})                                                                

we get

%!v(PANIC=String method: runtime error: invalid memory address or nil pointer dereference)

because when we're on the root item, it's children.At(-1) which returns nil, then we attempt to call Value() on it.

As-is we will always need to do a root value check in ItemStyleFunc to avoid this...

bashbunni commented 3 months ago

We might even want to see about separating out RootStyle from ItemStyle 💭

caarlos0 commented 3 months ago

We might even want to see about separating out RootStyle from ItemStyle 💭

i think that makes more sense yes