-
Notifications
You must be signed in to change notification settings - Fork 352
feat: tree bubble #639
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
base: v2-exp
Are you sure you want to change the base?
feat: tree bubble #639
Conversation
e32c664 to
0addf7c
Compare
tree/tree.go
Outdated
| // IsSelected returns whether this item is selected. | ||
| func (t *Node) IsSelected() bool { | ||
| return t.yOffset == t.opts.treeYOffset | ||
| } | ||
|
|
||
| // Depth returns the depth of the node in the tree. | ||
| func (t *Node) Depth() int { | ||
| return t.depth | ||
| } | ||
|
|
||
| // Size returns the number of nodes in the tree. | ||
| // Note that if a child isn't open, its size is 1 | ||
| func (t *Node) Size() int { | ||
| return t.size | ||
| } | ||
|
|
||
| // YOffset returns the vertical offset of the Node | ||
| func (t *Node) YOffset() int { | ||
| return t.yOffset | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These can be used to allow for more flexible styling.
I found the StyleFunc of lipgloss.Tree doesn't support some stuff I want, like appending a symbol to the end of each item at the same indentation to the right of it. Maybe there are more use cases.
tree/tree.go
Outdated
| } | ||
|
|
||
| // Used to print the Node's tree | ||
| // TODO: Value is not called on the root node, so we need to repeat the open/closed character |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I found I needed to implement the String() func to repeat the open/close character logic because the Value() func isn't called on the root. Maybe this is an easy fix in lipgloss.
tree/tree.go
Outdated
| func (t *Node) GivenValue() any { | ||
| return t.value | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was helpful in the TOC example I think. I found that the tree "swallows" my values and I can't read them again. This forces the user to save them before passing to the tree, which is just duplication and not the most ergonomic API. The name is confusing though given the Value() func also exists due to the Node interface from lipgloss.
A ValueFunc, similar to the StyleFunc could also solve this, as I believe that was my usecase.
tree/tree.go
Outdated
| func (t *Node) Children() ltree.Children { | ||
| return t.tree.Children() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This exposes lipgloss as it has to return lipgloss.Children since that's the interface
|
update: starting to take shape when used inside https://github.com/dlvhdr/diffnav
|
caarlos0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks great! would be awesome to add an example as well
021d11d to
dbf0223
Compare
|
I've just been playing with this PR locally and wondered whether it'd be possible to configure the open/closed character per node? The idea I had was a file tree where the closed character for a directory would be a closed folder and the open character would be an open folder. |
|
hey guys :) any update on this? |
|
Hey @dlvhdr sorry for the delay on this. We're going to spend some energy cleaning up bubbles before v2, so rest assured this will be merged soon |
|
This looks awesome! This will give better interactivity in its core essence. I think I'll make a lot of TUIs if this gets accepted. Thanks everyone for their efforts for making the SW world a better place! |
andreynering
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @dlvhdr,
This looks pretty solid, overall!
Some comments:
- Can you rebase with
v2-expand squash into a minimal number of commits? (Can be a single commit) - Is it easy enough to write some tests for this package?
- Ensure CI is green (lint included)
- You might need to temporarily pin the last commit from charmbracelet/lipgloss#446. Once merged, we'd pin from
v2-exp
- You might need to temporarily pin the last commit from charmbracelet/lipgloss#446. Once merged, we'd pin from
- I made a couple of comments below
tree/tree.go
Outdated
| // ScrollOff is the minimal number of lines to keep visible above and below the selected node. | ||
| ScrollOff int | ||
| // OpenCharacter is the character used to represent an open node. | ||
| OpenCharacter string | ||
| // ClosedCharacter is the character used to represent a closed node. | ||
| ClosedCharacter string | ||
| // CursorCharacter is the character used to represent the cursor. | ||
| CursorCharacter string | ||
| // KeyMap encodes the keybindings recognized by the widget. | ||
| KeyMap KeyMap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that in general, we'd like to avoid having public fields here as much as possible. So these keys like OpenCharacter and ClosedCharacter should be made private and additional Set* functions (SetOpenCharacter) should be implemented.
Am I right? @bashbunni @meowgorithm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, spot on. That will save is from having to change the API down the road.
dbf0223 to
46b545e
Compare
tree/tree.go
Outdated
| // KeyMap encodes the keybindings recognized by the widget. | ||
| KeyMap KeyMap | ||
| // styles sets the styling for the tree | ||
| styles Styles | ||
| Help help.Model |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move these two public fields (KeyMap and Help) to the top of the list, and add an empty line after to separate from the rest? The table.Table model also does this.
tree/tree.go
Outdated
| AdditionalShortHelpKeys func() []key.Binding | ||
| AdditionalFullHelpKeys func() []key.Binding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that these two should also be private with Set* functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed these, but just noted that these are public fields in the list component.
Let me know if you want me to revert / change the list as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still prefer to keep them private. It may have been by mistake that these got public on the other package.
go.mod
Outdated
| golang.org/x/text v0.23.0 // indirect | ||
| ) | ||
|
|
||
| replace github.com/charmbracelet/lipgloss/v2 v2.0.0-beta.1 => ../lipgloss |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I got that it's not possible to pin from your branch (as I asked) because it lives in your private fork.
It's fine to keep as is for now, and I can manually pin to the branch commit once I merge charmbracelet/lipgloss#446
andreynering
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work! 👏 👏 👏
Note that we're targeting v2.1.0 for this feature, so we're going to wait for v2.0.0 to be released before we merge your PRs.
|
Cool :) thanks for the reviews! |
|
@andreynering @dlvhdr any update on this? |
72d7eaa to
d3621ae
Compare
|
@andreynering any updates? |
|
I've just rebased the pr, it's ready for review |
|
@ossenthusiast As I said above, we're targeting v2.1.0 to ship this. We're already on a Release Candidate for v2.0.0, so the final release is near! Once released, we can do a final review and merge this. |
d3621ae to
1557de0
Compare

Introduces a tree bubble for TUIs that require a tree like model with expand/collapse/select functionality.
Screen.Recording.2024-10-25.at.17.31.24.mov
Intended Interactions
API
lipgloss.Treefor the renderingSelectedNode()SetSelectedNode()YOffsetin a getter so users can scroll to a nodedepthin a getterExample Usage
See charmbracelet/bubbletea#1190
Closes #233.