-
Notifications
You must be signed in to change notification settings - Fork 821
feat(tui): reduce animations #1250
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: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Carlos Alexandro Becker <[email protected]>
Signed-off-by: Carlos Alexandro Becker <[email protected]>
Signed-off-by: Carlos Alexandro Becker <[email protected]>
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.
The code looks good and works, but I think we can probably improve the UX a little bit, even if static.
internal/config/config.go
Outdated
| CompactMode bool `json:"compact_mode,omitempty" jsonschema:"description=Enable compact mode for the TUI interface,default=false"` | ||
| DiffMode string `json:"diff_mode,omitempty" jsonschema:"description=Diff mode for the TUI interface,enum=unified,enum=split"` | ||
| ReduceAnimations bool `json:"reduce_animations,omitempty" jsonschema:"description=Reduce animations in the TUI,default=false"` |
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.
We should eventually consider documenting these on the README. All of them are missing currently.
Maybe this is a good time to do it.
| func (s *noAnim) SetLabel(label string) { | ||
| s.rendered = lipgloss.NewStyle(). | ||
| Foreground(s.Color). | ||
| Render(cmp.Or(label, "Working") + ellipsisFrames[2]) |
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 think showing just a white Working... is a bit too boring. I wonder if we can improve it somehow. Perhaps add a colorful meaningful character as a prefix, at least to make it distinct from the actual text.
This is probably a job for @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.
Agreed—I can come up with some stuff.
Signed-off-by: Carlos Alexandro Becker <[email protected]>
Signed-off-by: Carlos Alexandro Becker <[email protected]>
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.
Code LGTM
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.
Let's update the README to expose the config + what do you think about adding an environment variable as well @caarlos0 and @andreynering (or is SSH_TTY enough)?
I'll also come up with some options for "Working…".
internal/tui/components/anim/anim.go
Outdated
| // If the FPS is 20 (50 milliseconds) this means that the ellipsis will | ||
| // change every 8 frames (400 milliseconds). | ||
| ellipsisAnimSpeed = 8 | ||
| ellipsisanimSpeed = 8 |
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.
Looks like you mistakenly reverted a rename here.
|
No strong opinion on the env. It should be simple to add, so why not? Not sure on the name:
|
Signed-off-by: Carlos Alexandro Becker <[email protected]>
Signed-off-by: Carlos Alexandro Becker <[email protected]>
|
@meowgorithm done |
Signed-off-by: Carlos Alexandro Becker <[email protected]>
This allows to disable the spinner completely.
Instead, it'll show a static text while working, and then disappear.
closes #908
closes #1147