-
Notifications
You must be signed in to change notification settings - Fork 228
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
Adding a router bubble for navigation #326
base: master
Are you sure you want to change the base?
Conversation
@maaslalani I've developed a rough prototype for a router bubble (Closes #179). Still needs a lot of improvements like:
Would love to discuss this with you! (maybe over discord?) |
Yes, let's definitely discuss this over discord! Thank you so much for this initiative, it will change the way we build multi-screen TUIs and set much better standards into place. I have a bit of feedback and a few ideas on how we can make this super solid! |
|
||
// Init implements tea.Model | ||
func (Model) Init() tea.Cmd { | ||
return nil |
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 wonder if we should Init all the screens here?
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, makes sense, will do!
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.
@maaslalani I was wondering if I should only init the current screen or all of them.
Because in case we do all of them, how do we handle the return messages?
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 you're right, It probably makes sense to Init
the current screen, we should probably keep track of whether the screen has been initialized and do that on first load (we should check this every time the navigator switches screens).
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.
Makes sense, so it'll be kind of lazily executed Init
. If there's a need for something to be executed right away in the future, we can extend the api to accomodate
@maaslalani done with the initialisation logic, how do you recommend handling invalid |
@maaslalani are there any changes or should I start working on documentation for the bubble? |
Hey @KaviiSuri, I will need to give this more thought to make sure it can fit all use cases we have for navigation. But, this is an awesome start. Thank you so much for this contribution. |
One thing I did think of is we probably will want to make the screens a const (
homeScreen = "home"
settingsScreen = "settings"
)
// Somewhere else in the application...
navigator.Navigate(homeScreen)
navigator.Push(settingsScreen) |
@maaslalani Yupp, makes sense, will come up with something soon. |
@maaslalani I've added a history stack to thr router. P.S. Sorry for the delay, had some stuff going on |
Hey @KaviiSuri, thanks again for this PR. I'm going to schedule a bit of time later on to play with this and then we can get this merged in to the experimental package: So that others can play with it and try it out themselves so we can get some feedback before making it official. |
This PR Adds a
router
bubble to make developing multi-screen bubbletea apps easier.This is just a prototype!