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: change terminal time stamp color from bug bright red to improve readibility #708

Conversation

ve-varun-sharma
Copy link
Contributor

Pull Request Type

Dictionary of Definitions here:
https://ben.balter.com/2015/12/08/types-of-pull-requests/

  • [] Just a heads up πŸ™†
  • Sanity check πŸƒβ€β™‚οΈ
  • Work in progress (WIP) πŸ—οΈ
  • Early feedback ✍️
  • Line-by-line review πŸ“
  • Pull request to a pull request πŸ”€

Description

The terminal color change should improve Readability, reduced Confusion via by avoiding the use of red for timestamps, we can minimize the risk of false positives and streamline the debugging process, and overall abide by terminal UX best practices from what I understand about the objective of the time stamp purpose in the defang project.

This PR addresses this Github issue: #707

Caveats/Notes (optional)

I was not able to fully test the terminal UI as I did not run this specific defang version. Instructions on how to best run a local emulated version from VS code would be fantastic too!

Prompts (optional)

Screenshots (optional)

[Upload optional screenshots]

Checklist before requesting a review

  • I have performed a self-review of my code
  • If it is a core feature, I have added thorough tests.
  • Extended the README / documentation, if necessary
  • My changes generate no new warnings
  • Is the code readable? Prefer to see more self-explanatory and consistency
  • Is the code tested properly? i.e. where appropriate, we're testing well-explained failures, behaviours, fixes, etc

@ve-varun-sharma ve-varun-sharma changed the title feat: change terminal time stamp color from bug brigth red to blue feat: change terminal time stamp color from bug bright red to improve readibility Sep 30, 2024
@lionello
Copy link
Member

lionello commented Oct 1, 2024

I agree there's improvements to be made here. Timestamps are Red for error logs, which are the only ones you'll see when you omit --verbose.

@lionello lionello added the question Further information is requested label Oct 1, 2024
@@ -331,7 +331,7 @@ func tail(ctx context.Context, client client.Client, params TailOptions) error {
tsColor = termenv.ANSIWhite
}
if e.Stderr {
tsColor = termenv.ANSIBrightRed
tsColor = termenv.ansiCyan
Copy link
Member

@lionello lionello Oct 1, 2024

Choose a reason for hiding this comment

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

It's red when the log comes from stderr but maybe the logs you're seeing shouldn't be errors?

@lionello
Copy link
Member

lionello commented Nov 4, 2024

@ve-varun-sharma closing this because we did make some changes around our verbosity. Also "cyan" was already used for another column. I think there's something to be said to make timestamps optional, then this would not be an issue.

@lionello lionello closed this Nov 4, 2024
@ve-varun-sharma
Copy link
Contributor Author

@ve-varun-sharma closing this because we did make some changes around our verbosity. Also "cyan" was already used for another column. I think there's something to be said to make timestamps optional, then this would not be an issue.

That definitely makes sense to me! Thanks for the update here :) πŸ™πŸ½

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants