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

Adding a router bubble for navigation #326

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

KaviiSuri
Copy link

This PR Adds a router bubble to make developing multi-screen bubbletea apps easier.

This is just a prototype!

@KaviiSuri
Copy link
Author

@maaslalani I've developed a rough prototype for a router bubble (Closes #179). Still needs a lot of improvements like:

  • Stack based routing (basically keeping track of back and using it)
  • Help text
  • Better helper methods and design

Would love to discuss this with you! (maybe over discord?)

@maaslalani
Copy link
Member

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

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?

Copy link
Author

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!

Copy link
Author

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?

Copy link
Member

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).

Copy link
Author

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

router/router.go Outdated Show resolved Hide resolved
@KaviiSuri
Copy link
Author

@maaslalani done with the initialisation logic, how do you recommend handling invalid m.current

@KaviiSuri KaviiSuri marked this pull request as ready for review February 27, 2023 18:16
@KaviiSuri
Copy link
Author

@maaslalani are there any changes or should I start working on documentation for the bubble?

@maaslalani
Copy link
Member

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.

@maaslalani
Copy link
Member

maaslalani commented Mar 6, 2023

One thing I did think of is we probably will want to make the screens a map[string]tea.Model / map[string]Screen so that each screen has an associated id such that you can navigate to screens / models directly using some helper function.

const (
  homeScreen = "home"
  settingsScreen = "settings"
)

// Somewhere else in the application...
navigator.Navigate(homeScreen)
navigator.Push(settingsScreen)

@KaviiSuri
Copy link
Author

@maaslalani Yupp, makes sense, will come up with something soon.

@KaviiSuri
Copy link
Author

@maaslalani I've added a history stack to thr router.

P.S. Sorry for the delay, had some stuff going on

@maaslalani
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants