-
Notifications
You must be signed in to change notification settings - Fork 93
Issue #97, add explanation box - 2nd PR #454
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
Conversation
|
I didn't know that I could update the commits in a PR without creating a new one. Sorry about the duplicated PR. |
La0
left a comment
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.
Great work !
@marco-c I'm ok with the display code, even though the style is a bit overkill, it looks nice.
We could reduce the explanation text as it's simply a copypasta from an IRC discussion right now. WDYT ?
|
And here is a link to the frontend from this build so you can test. |
|
Yes, definitely better to rewrite that:
|
|
@InfTkm Could you update the explanation text with the one mentionned above by Marco ? |
La0
left a comment
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.
Thanks for this great contribution 👍
Fixed #97 and deleted trailing white spaces according to Taskcluster's log.
https://youtu.be/xmY6yLb7gu4
Here is a demo. Hopefully, I understood correctly where the question mark box should be.
Explanation of what I have done:
Line 59 in zero_coverage_report.js: sets the variable needs_explanation to true for the filter, completely_uncovered.
Line 138 - 146 in base.html: add the tooltip if the filter needs explanation (i.e. completely_uncovered).
At the end of style.scss: I added the CSS for the tooltip (i.e. popup explanation box) using SASS format.
Let me know if you have any questions or feedback. Thanks.