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

Update MicroCloud look and feel #505

Merged
merged 15 commits into from
Dec 13, 2024
Merged

Update MicroCloud look and feel #505

merged 15 commits into from
Dec 13, 2024

Conversation

masnax
Copy link
Contributor

@masnax masnax commented Nov 20, 2024

Uses https://github.com/charmbracelet/lipgloss to implement tables and questions in MicroCloud.

  • There is much more colour in the CLI now. This can be disabled with the NO_COLOR environment variable or the --no-color flag.

  • make build-test has been added to the Makefile, which builds MicroCloud with different input/output sources to work with the system tests. TEST_CONSOLE=1 only works with this build tag now.

  • the test tag also replaces the EFF wordlist with a predictable 'a a a a' to reduce complexity in the test script and make debugging easier.

  • package dependencies for the old implementation and test console have been removed.

  • there is a new file at /var/snap/microcloud/common/test-output that is generated with the test build tag and TEST_CONSOLE=1 to show how each question interpreted the pre-defined input.

@masnax masnax force-pushed the tui branch 10 times, most recently from 49e24dc to 75238eb Compare November 21, 2024 18:19
@masnax masnax requested a review from roosterfish November 21, 2024 20:37
Copy link
Contributor

@roosterfish roosterfish left a comment

Choose a reason for hiding this comment

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

Thanks for this, looks very good and makes fun playing around with the new UX additions. Also the table navigation is very clean.

I have commented on a few places about minor things but will put some general comments to the new colors/font weight here.

I like the fact that the users input (as well as the default values throughout the questionnaire) is now highlighted using bold text. But also marking it yellow feels a bit too much. I am having a hard time reason about the yellow for such types of inputs if other (good) things are marked green and errors are in red. But that's personal preference I guess.
Just using bold text would be enough I think.
Then colors are really dedicated for errors, actual warnings (yellow) and security related notes (fingerprints, passphrases) in green.

cmd/tui/asker.go Show resolved Hide resolved
cmd/tui/asker.go Show resolved Hide resolved
cmd/tui/asker.go Show resolved Hide resolved
cmd/tui/handler.go Outdated Show resolved Hide resolved
cmd/tui/handler.go Outdated Show resolved Hide resolved
cmd/microcloud/test_input.go Outdated Show resolved Hide resolved
cmd/microcloud/ask.go Show resolved Hide resolved
Makefile Show resolved Hide resolved
service/test_wordlist.go Outdated Show resolved Hide resolved
cmd/microcloud/cluster_members.go Show resolved Hide resolved
@roosterfish
Copy link
Contributor

Something else I just came across, can we maybe introduce something like a "note" to display messages like Do not exit out to keep the session alive!? Printing them in bold red gives you the feeling there is an error.

To keep it simple maybe just a colored box like in the example here (using a more modest color though)

@masnax
Copy link
Contributor Author

masnax commented Nov 29, 2024

Something else I just came across, can we maybe introduce something like a "note" to display messages like Do not exit out to keep the session alive!? Printing them in bold red gives you the feeling there is an error.

To keep it simple maybe just a colored box like in the example here (using a more modest color though)

Interesting idea to have a Note option. The goal is to not introduce more colours to maintain high compatibility with as many terminal themes as possible, but a yellow/warning box should suffice I think.

@masnax
Copy link
Contributor Author

masnax commented Dec 3, 2024

I like the fact that the users input (as well as the default values throughout the questionnaire) is now highlighted using bold text. But also marking it yellow feels a bit too much. I am having a hard time reason about the yellow for such types of inputs if other (good) things are marked green and errors are in red. But that's personal preference I guess. Just using bold text would be enough I think. Then colors are really dedicated for errors, actual warnings (yellow) and security related notes (fingerprints, passphrases) in green.

The reason for the colouring is two-fold:

  • To distinguish it from the immediately preceding component of the question which is already in bold. This is the default value like default=yes.
  • To easily distinguish user-input from question text at a glance.

Yellow basically just means "look at this", but does not signify any problem that must be addressed for the cluster to form.

@masnax masnax force-pushed the tui branch 6 times, most recently from 6097eea to 217fed2 Compare December 5, 2024 19:22
@masnax
Copy link
Contributor Author

masnax commented Dec 5, 2024

Something else I just came across, can we maybe introduce something like a "note" to display messages like Do not exit out to keep the session alive!? Printing them in bold red gives you the feeling there is an error.

To keep it simple maybe just a colored box like in the example here (using a more modest color though)

I've added a tui.Note helper now that prints a bordered box with text inside. In the case of this message, the box border is set to yellow, and the text is white, with a yellow exclamation at the front.

Copy link
Contributor

@roosterfish roosterfish left a comment

Choose a reason for hiding this comment

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

Thanks for adding the note box, looks very good and helpful in my opinion!

Just did some more deploys with the changes from this PR and thought the storage disk error for single node deployments should maybe also be put into such a box for consistency? Another candidate might be the subnet collision warning. This could already give a hint for the changes in #522.

@masnax
Copy link
Contributor Author

masnax commented Dec 9, 2024

Thanks for adding the note box, looks very good and helpful in my opinion!

Just did some more deploys with the changes from this PR and thought the storage disk error for single node deployments should maybe also be put into such a box for consistency? Another candidate might be the subnet collision warning. This could already give a hint for the changes in #522.

Couple ways to go about this:

  1. Make every retryable error message print in a box. That would be consistent but might make the CLI history messy and hard to read.

  2. Make this specific message print in a box. We would lose the red X from the retry sequence if we did this, since the retry message would need to be printed as an unedited string. It would also be less consistent, but less noisy.

  3. Make the whole retry sequence including the question component print in a box. This would let us clean up the box after the question has been answered, but the user would not be able to keep the history of this input.

I'll give each version a try and see what looks best.

@roosterfish
Copy link
Contributor

@masnax ready for final review? I saw you pushed some more changes. And your commits look to be partially unverified.

@masnax
Copy link
Contributor Author

masnax commented Dec 12, 2024

Ah shoot, I switched ssh keys for launchpad and broke github. I'll fix that :)

The changes were just a bugfix around TEST_CONSOLE still having functionality when the test build tag is not set. Now it should do nothing if MicroCloud is built for production.

Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
@masnax
Copy link
Contributor Author

masnax commented Dec 12, 2024

@roosterfish Commits are fixed now.

Copy link
Contributor

@roosterfish roosterfish left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@roosterfish roosterfish merged commit 563c565 into canonical:main Dec 13, 2024
22 checks passed
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