-
Notifications
You must be signed in to change notification settings - Fork 14
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
Add some more user feedback #97
Conversation
94e88d5
to
87df0ae
Compare
Need some review on the copy added to make sure that it is accurate @blu3beri 😅 |
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.
Nice addition, just some minor code cleanup and it will be merged :D
b3ec912
to
e89ef43
Compare
ConfigurationSubcommands::View => { | ||
debug!( | ||
"Loaded configuration from {}", | ||
String::from(config_path.to_str().unwrap()).bold() |
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.
String::from is not required here right?
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.
This is so that we can use bold()
1a7d6cb
to
b61b784
Compare
@blu3beri I think I've address your feedback, would you mind taking another look? |
Small PR adding a few more points of user feedback. Wanted to add these before I forgot.
Side notes
While adding this I was running into an issue creating a credential definition. Feedback:
This is actually OK feedback, but it would be really nice if we told users what they could do to fix the issue, not just that there is an issue.
Screenhots