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: move tree to root #342

Merged
merged 12 commits into from
Aug 19, 2024
Merged

feat: move tree to root #342

merged 12 commits into from
Aug 19, 2024

Conversation

caarlos0
Copy link
Member

closes #339
closes #341

bashbunni and others added 3 commits July 23, 2024 17:20
Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
@caarlos0 caarlos0 added the enhancement New feature or request label Jul 23, 2024
@caarlos0 caarlos0 requested a review from bashbunni July 23, 2024 22:11
@caarlos0 caarlos0 self-assigned this Jul 23, 2024
@meowgorithm
Copy link
Member

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

@bashbunni
Copy link
Member

Hey @meowgorithm any changes on this one?

@meowgorithm
Copy link
Member

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

@bashbunni
Copy link
Member

@meowgorithm kk no rush. Just added the examples back

Copy link
Member

@meowgorithm meowgorithm left a comment

Choose a reason for hiding this comment

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

Feeling good! Let's get this done ⚡️

@bashbunni
Copy link
Member

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 bashbunni added this to the v0.13.0 milestone Aug 5, 2024
Copy link
Member

@bashbunni bashbunni left a comment

Choose a reason for hiding this comment

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

TODO before merge

  • tidy godoc (some tree docs mention list)
  • tidy godoc - opened a PR against this branch with improvements
  • show an example of unique styling of the root element (this might be broken)
  • add tree info to README.md

@bashbunni
Copy link
Member

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
Copy link
Member

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

@caarlos0
Copy link
Member Author

caarlos0 commented Aug 6, 2024

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

i think that makes more sense yes

README.md Outdated Show resolved Hide resolved
@caarlos0 caarlos0 merged commit feb42a9 into master Aug 19, 2024
20 checks passed
@caarlos0 caarlos0 deleted the bring-tree-back-v2 branch August 19, 2024 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mixed up use of Top/Left Bottom/Right in JoinHorizontal and JoinVertical calls How to create trees
4 participants