Update coverage page styles #41
Conversation
| <div class="linuxfoundation-footer"> | ||
| <div class="container"> | ||
| <a class="linuxfoundation-logo" href="http://collabprojects.linuxfoundation.org">Linux Foundation Collaborative Projects</a> | ||
| <p>© 2016 Node.js Foundation. All Rights Reserved. Portions of this site originally © 2016 Joyent. </p> |
There was a problem hiding this comment.
Changing this to © may work in this case but it’s not really fixing anything, just working around the underlying problem… I can’t reproduce the problems locally, so I assume it’s not related to the script itself but to the surrounding setup; @mhdawson might know more?
There was a problem hiding this comment.
I'd copied the footer from https://github.com/nodejs/benchmarking/blob/master/www/index.html so the difference may be in writing out the stings. In the benchmarking case the footer is just part of the index file, while in the coverage case we write it out from Node.js I'll have to look closer.
There was a problem hiding this comment.
The problems seems to be this line:
<meta charset="utf-8">
I saved the contents from coverage.nodejs.org to a local file, opened it in the browser and saw the mangled characters. I then commented out that line and they looked ok.
There was a problem hiding this comment.
@mhdawson Yeah, the problem is obviously that the file is transferred as ISO-8859-1. The question is, why is that? 😄
There was a problem hiding this comment.
Seems though that the benchmarking page has the same header. Looking at the files when I download them, the difference may be an extra character so what I think it happening is that some sort of escape is being lost since we print out the string as opposed to it just being in the file.
Having said all that I think instead of figure out how to escape that character, the @copy; is much clearer when looking at the source so I'd prefer to go with that.
There was a problem hiding this comment.
@addaleax, the file looks the same to me on the machine where it was generated, so I don't think it has anything to do with the transfers, just how it ends up being generated. I see © in the bechmarking index.html while what we end up with is © in the one for code coverage. Looking in generate-index-html.py I also only see © so it may be in my cut paste that got lost. I'm still liking @copy; better in terms of readabiliyt.
There was a problem hiding this comment.
I see © in the bechmarking index.html
Yeah, that’s an UTF-8 © sign read as ISO-8859-1. Is the terminal on the machine set up to display ISO-8859-1?
I mean I guess this is okay to do with ©, this page likely won’t ever contain a lot of text…
|
Maybe one additional idea: Should the git SHA be displayed in a monospace font? |
|
@addaleax Love that idea. Pushed the change. I just used the same font-families as github. |
mhdawson
left a comment
mhdawson
left a comment
There was a problem hiding this comment.
LGTM. I applied the changes and ran a test generation. Looks good.
|
Is it possible to move this out of the python script? |
|
@Fishrock123 Do you mean separate the HTML from python? Sure, I could do that. |
|
@ryanmurakami I think that would be cool, yes. This is just something I hacked together, so if you’re willing to spend time on improvements, I’m definitely grateful! |
|
I agree separating is a good thing as well. Talked with @Fishrock123 and will land this as is to get the fixups live. @ryanmurakami if you can then submit a follow on PR to do the separation that would be great :) |
- fixing copyright symbols - changing styling and formatting for page - using monospace font for sha PR-URL: #41 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
|
Landed as e0c9bc6 |
|
Run to validate job still runs/works ok (I expect it will since I manually applied to validate, but better safe than sorry) https://ci.nodejs.org/view/All/job/node-test-commit-linux-coverage/ |
|
@ryanmurakami thanks for your help, and looking forward to the PR to split out as well. |
|
@mhdawson Sounds good, thanks! Any reason why it's in Python? In objections if I switch over to Node? |
No particular reason besides me finding Python a bit easier for simple text processing; I don’t think there’s any real reason not to switch to Node. |
|
@addaleax Okay cool. Thanks for sharing! :-) |
|
One reason is that the build machines don't necessarily have node installed while they all have python. @jbergstroem what is your take on adding a dependency on node for this. ? Right now it would be easy since we only have it running on a single machine but if we can get to the point where it can run on any machine it might be more of an issue. |
|
@mhdawson We could use the node executable built during testing for coverage, right? |
|
There have been discussions that we'd not want to do that when its been considered before (mostly around that you are trying to test something that may or may not be fully functional so you don't necessarily want to depend on that for the test itself to work - that may not apply as directly here). I'm also thinking that I might pull that step out of the job itself as it currently means the history is on the machine were we run as opposed to the data machine which would mean we'd still need to add it as a dependency on some machines (the data machine at least). |
|
I'm totally fine with python, just thought Node made more sense, y'know? :-) Sounds like python is a better choice here, though, due to some ops complications. |
|
Live and looking good: https://coverage.nodejs.org/. Closing |
4 participants
Updating some styling and formatting on the cover page. This is was suggested from nodejs/nodejs.org#1019
Here are some of the changes:
Here's a screenshot on Chrome:
