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

Polish UX for sandbox commands #4

Merged
merged 11 commits into from
May 21, 2022
Merged

Polish UX for sandbox commands #4

merged 11 commits into from
May 21, 2022

Conversation

@enisoc enisoc requested a review from scott-cotton May 18, 2022 16:17
@scott-cotton
Copy link
Member

For delete, I am getting the attached image for the output, which is odd because it is
repeating the same (truncated) message but still has the spinner.

Screenshot 2022-05-19 at 16 33 56

@enisoc
Copy link
Contributor Author

enisoc commented May 19, 2022

I was able to reproduce that too by making my terminal window narrower. So it seems that the spinner library (yacspin) doesn't work properly when you try to print a message that doesn't fit in the terminal.

I don't see an issue for that library, but the other popular Go spinner library seems to have the same issue. It appears to be a common challenge for spinners, though potentially solvable.

As a quick workaround, perhaps we could truncate the message to fit in the terminal while the spinner is running, and then print the full message only at the end when the spinner stops.

@enisoc
Copy link
Contributor Author

enisoc commented May 19, 2022

@scott-cotton I implemented the above workaround. PTAL.

Copy link
Member

@scott-cotton scott-cotton left a comment

Choose a reason for hiding this comment

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

Regarding the print package, it seems to me that it is starting to scatter the consolidation of a command implementation under internal/command/

Would it be possible to have things like print.{ClusterTable, SandboxDetails} in its own file and function under internal/command/? In this scenario I would consider what's remaining in print to be minor helper functionality such as making the json more like the yaml, and it seems it would retain the nice property of the current organisation in this PR of breaking out individual cases when they start to become more cumbersome in the switch statement.

@scott-cotton
Copy link
Member

@scott-cotton I implemented the above workaround. PTAL.

Looks like that will work, great. At the same time, I'd personally have preferred being more conservative about the packages we import, as often enough the dependencies + such workarounds end up taking more time in the long run than building directly what we need, starting with minimal features. A spinner seems like a it may be such a case.

@enisoc
Copy link
Contributor Author

enisoc commented May 20, 2022

Regarding the print package, it seems to me that it is starting to scatter the consolidation of a command implementation under internal/command/

Good point. I moved the command-specific printers back into the same package as the rest of each command's implementation.

A spinner seems like a it may be such a case.

I thought so too so I started without a spinner at all and just trying to rewrite the status line. I started running into terminal nuances and at some point I decided it would be a better use of my time to just pick an off-the-shelf library because I don't know enough about this terminal magic.

@enisoc enisoc requested a review from foxish May 20, 2022 21:17
Copy link
Member

@foxish foxish left a comment

Choose a reason for hiding this comment

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

LGTM aside from the one comment

}
status := result.Payload.Status
if status.Ready {
spin.Message("Waiting for sandbox to begin terminating.")
Copy link
Member

Choose a reason for hiding this comment

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

seems like a weird error message - maybe we group this under "Waiting for sandbox to terminate"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@enisoc enisoc merged commit 424bbad into main May 21, 2022
@enisoc enisoc deleted the enisoc-sandbox-ux branch May 21, 2022 00:03
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.

3 participants