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

🎨 [Frontend] Friendlier logger #6907

Merged
merged 33 commits into from
Dec 9, 2024

Conversation

odeimaiz
Copy link
Member

@odeimaiz odeimaiz commented Dec 5, 2024

What do these changes do?

requested by @AntoninoMarioC

  • Do not color messages (warning and error), add an icon instead
  • Make selected line's background color less prominent
  • Interpret the "\n" from the log message

After:
Logger

Before:
LoggerBefore

Related issue/s

How to test

Dev-ops checklist

@odeimaiz odeimaiz self-assigned this Dec 5, 2024
@odeimaiz odeimaiz marked this pull request as ready for review December 6, 2024 12:46
@odeimaiz odeimaiz added t:enhancement Improvement or request on an existing feature a:frontend issue affecting the front-end (area group) labels Dec 6, 2024
@odeimaiz odeimaiz added this to the Event Horizon milestone Dec 6, 2024
@odeimaiz odeimaiz enabled auto-merge (squash) December 6, 2024 12:53
@mguidon
Copy link
Member

mguidon commented Dec 6, 2024

Nice. Why not using the same color for the error as in the Failed Status of the nodes?

@odeimaiz
Copy link
Member Author

odeimaiz commented Dec 6, 2024

Nice. Why not using the same color for the error as in the Failed Status of the nodes?

Yes, it's there. I recorded the animation right before realizing the error icon was yellow 😬

Copy link
Member

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

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

Nice.

I have a suggestion. I undrestand that the filter means >= log_level

image

since it shows everything above the value you select.
I would make that UX more clear and I would alos add the possibilty of having ONLY one level at a time

@odeimaiz
Copy link
Member Author

odeimaiz commented Dec 6, 2024

Nice.

I have a suggestion. I undrestand that the filter means >= log_level

image

since it shows everything above the value you select. I would make that UX more clear and I would alos add the possibilty of having ONLY one level at a time

I don't like it. When was the last time you were interested in the INFO messages but not in the ERROR messages?

Furthermore, I would need to add a new option, SHOW ALL which would be default, and this Show all would behave differently depending on the user's role, because we don't show the Debug messages to non-tester users... An effort which I believe it's not worth.

@pcrespov
Copy link
Member

pcrespov commented Dec 6, 2024

I don't like it. When was the last time you were interested in the INFO messages but not in the ERROR messages?

I am not suggesting only to have all options available but rather than the UX is confusing.

If i select in a filter INFO, i was expecting to filter only the values that are marked as INFO but what the UI does is showing "at least INFO". If that is the case, IMO the UX should change to indicate so. I am not sure how, ... perhaps adding "show at least.. ."? or "min log-level" ... or writing ">=" ?? Not sure

Furthermore, I would need to add a new option, SHOW ALL which would be default, and this Show all would behave differently depending on the user's role, because we don't show the Debug messages to non-tester users... An effort which I believe it's not worth.

As I mentioned earlier, my comment is more on the UX rather than the availability of options.

@odeimaiz
Copy link
Member Author

odeimaiz commented Dec 6, 2024

I don't like it. When was the last time you were interested in the INFO messages but not in the ERROR messages?

I am not suggesting only to have all options available but rather than the UX is confusing.

If i select in a filter INFO, i was expecting to filter only the values that are marked as INFO but what the UI does is showing "at least INFO". If that is the case, IMO the UX should change to indicate so. I am not sure how, ... perhaps adding "show at least.. ."? or "min log-level" ... or writing ">=" ?? Not sure

Furthermore, I would need to add a new option, SHOW ALL which would be default, and this Show all would behave differently depending on the user's role, because we don't show the Debug messages to non-tester users... An effort which I believe it's not worth.

As I mentioned earlier, my comment is more on the UX rather than the availability of options.

Added Min log-level text to make it less confusing 👍

Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

very nice!

Copy link
Contributor

@matusdrobuliak66 matusdrobuliak66 left a comment

Choose a reason for hiding this comment

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

🥇

Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

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

👍

Copy link

sonarqubecloud bot commented Dec 9, 2024

@odeimaiz odeimaiz merged commit 661f229 into ITISFoundation:master Dec 9, 2024
55 checks passed
@odeimaiz odeimaiz deleted the enh/friendlier-logger branch December 9, 2024 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:frontend issue affecting the front-end (area group) t:enhancement Improvement or request on an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants