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

feat: show success message after creating a flag #134

Merged

Conversation

k3llymariee
Copy link
Contributor

@k3llymariee k3llymariee commented Apr 4, 2024

Adds an interstitial step to show the user that either a new flag was created, or an existing one was used, during the first step of the setup command.

new-flag

existing-flag

Copy link

This pull request has been linked to Shortcut Story #239299: show success message after creating a flag.

Comment on lines 19 to 21
existingFlagUsed bool
flagKey string
success bool
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are the new fields - also storing the flag key now to pass into the view

@@ -58,6 +61,14 @@ func (m createFlagModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
default:
m.textInput, cmd = m.textInput.Update(msg)
}
case createdFlagMsg:
if !m.success {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't love this name so open to suggestions

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does "success" mean that the user created a new flag or tried to create an existing flag so that we would (over)write the flag key with the one from the message? Maybe instead of "success" it could be something like "shouldUpdateFlag"?

if m.success {
successMessage := fmt.Sprintf("Flag %q created successfully!", m.flagKey)
if m.existingFlagUsed {
successMessage = fmt.Sprintf("Using existing existing flag %q for setup.", m.flagKey)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stutter with "existing."

@@ -67,6 +78,14 @@ func (m createFlagModel) View() string {
style := lipgloss.NewStyle().
MarginLeft(2)

if m.success {
successMessage := fmt.Sprintf("Flag %q created successfully!", m.flagKey)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use the flag name instead of the key since that's what the user typed? Otherwise they could be surprised if they type "my flag" and we show "my-flag." If we go that route it may be simpler to create a flag struct for the key and name instead of passing both around.

@@ -58,6 +61,14 @@ func (m createFlagModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
default:
m.textInput, cmd = m.textInput.Update(msg)
}
case createdFlagMsg:
if !m.success {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does "success" mean that the user created a new flag or tried to create an existing flag so that we would (over)write the flag key with the one from the message? Maybe instead of "success" it could be something like "shouldUpdateFlag"?

Comment on lines +88 to +94
if m.showSuccessView {
successMessage := fmt.Sprintf("Flag %q created successfully!", m.flag.name)
if m.existingFlagUsed {
successMessage = fmt.Sprintf("Using existing flag %q for setup.", m.flag.name)
}
return successMessage + " Press enter to continue."
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbolson updated the name here - is this doing too much/do I need another message type here to pass around?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, maybe we should have a message for when we create a new flag and one where the flag already exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I meant for this comment to go on the Update step where I was overloading one message type, but think I've managed to fix

Comment on lines +88 to +94
if m.showSuccessView {
successMessage := fmt.Sprintf("Flag %q created successfully!", m.flag.name)
if m.existingFlagUsed {
successMessage = fmt.Sprintf("Using existing flag %q for setup.", m.flag.name)
}
return successMessage + " Press enter to continue."
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I meant for this comment to go on the Update step where I was overloading one message type, but think I've managed to fix

Comment on lines 17 to 21
type flagDetail struct {
key string
name string
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added new flagDetail type

Comment on lines +72 to +74
// can only go back if a flag has been created but not confirmed,
// so let the model handle the Update
m.currentModel, cmd = m.currentModel.Update(msg)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is new also, to allow going back after a flag has been created (but not before)

@k3llymariee k3llymariee requested a review from dbolson April 8, 2024 22:48
m.showSuccessView = true
m.existingFlagUsed = msg.existingFlagUsed
m.flag = msg.flag
return m, cmd
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this needs to return early.

@@ -14,14 +14,22 @@ import (

const defaultFlagName = "My New Flag"

type flagDetail struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could probably only need to call this flag.

@k3llymariee k3llymariee merged commit f817856 into main Apr 9, 2024
2 checks passed
@k3llymariee k3llymariee deleted the kelly/sc-239299/show-success-message-after-creating-a-flag branch April 9, 2024 00:27
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.

2 participants