-
Notifications
You must be signed in to change notification settings - Fork 205
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 interactive log viewer #5836
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
@asmorodskyi WDYT? |
This comment was marked as resolved.
This comment was marked as resolved.
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.
Looks generally good. I only have a few suggestions regarding the JavaScript code.
Thanks for demo ! LGTM ! This is certainly step in right direction ! |
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.
LGTM
The feature seems to work like I want it now. Feel free to try it out. And I still need to write a test. |
933fdc9
to
3e0a7f5
Compare
Need to tidy |
I added the test and other small fixes. Reviews welcome |
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.
I haven't tested it yet but it looks generally good and I see you have implemented my suggestion. Now I only have a few coding-style-related remarks.
regex = new RegExp(match[1], match[2]); | ||
} | ||
displaySearchInfo('Searching…'); | ||
$('.embedded-logfile').each(function (index, logFileElement) { |
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.
I'd avoid using jQuery in new code. This could be written using plain JavaScript like this:
$('.embedded-logfile').each(function (index, logFileElement) { | |
Array.from(document.getElementsByClassName('embedded-logfile')).forEach((logFileElement) => { |
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.
It's using the same code as in the existing loadEmbeddedLogFiles
and it would have been weird to use one style in one function and one in the other. And when looking for removing jquery from the existing code, how far should I go?
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.
You can keep it. I'd just avoid it in new code even though it means we don't have a consistent style everywhere.
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.
Oh, and you can actually avoid the outer Array.from(…)
in my example. The node list should also have a forEach
method.
Updated style suggestions |
60b08e6
to
b76b60c
Compare
This comment was marked as resolved.
This comment was marked as resolved.
One can now filter the log files for substring and regexes. Issue: https://progress.opensuse.org/issues/162218
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5836 +/- ##
=======================================
Coverage 98.50% 98.50%
=======================================
Files 395 395
Lines 38759 38767 +8
=======================================
+ Hits 38180 38188 +8
Misses 579 579 ☔ View full report in Codecov by Sentry. |
One can now filter the log files for substring and regexes.
Demo: https://github.com/user-attachments/assets/f6dec605-951a-4e14-8f93-18acd461a708
Issue: https://progress.opensuse.org/issues/162218