-
Notifications
You must be signed in to change notification settings - Fork 1
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: Can go back to choose SDK page from show SDK instructions #120
Conversation
This pull request has been linked to Shortcut Story #239052: go back from choose SDK step in setup. |
flagsClient flags.Client | ||
quitMsg string // TODO: set this? | ||
quitting bool | ||
sdk sdkDetail |
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.
Added this and sorted the others.
cmd = m.currentModel.Init() | ||
m.sdkKind = msg.sdkKind | ||
m.sdk = msg.sdk |
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'm passing the entire sdk details instead of its fields.
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.
just one comment about the key we're using but otherwise LGTM
internal/quickstart/container.go
Outdated
Back: key.NewBinding( | ||
key.WithKeys("left"), | ||
key.WithHelp("back", "go back"), | ||
), |
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.
is it weird to use left
here when we were just using it to traverse the the list in the previous step? do we not want to use esc
?
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.
Updated
internal/quickstart/container.go
Outdated
m.currentStep += 1 | ||
case selectedSDKMsg: | ||
m.currentModel, cmd = m.currentModel.Update(msg) |
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.
could this be grouped with the case statement for the msg's on line 95?
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.
Updated
d698e6f
to
3136db5
Compare
A user can go back to choose another SDK once they're on the instructions page. We should show this in the help text once we have that.
Requirements
Related issues
Provide links to any issues in this repository or elsewhere relating to this pull request.
Describe the solution you've provided
Provide a clear and concise description of what you expect to happen.
Describe alternatives you've considered
Provide a clear and concise description of any alternative solutions or features you've considered.
Additional context
Add any other context about the pull request here.