-
Notifications
You must be signed in to change notification settings - Fork 53
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 generating downloads from JSON #187
Conversation
Partially implements Tox#120 - downloads are now generated from a JSON file - generating download buttons for all GNU/Linux distros with JavaScript - small improvements to code readability in osdetect.js - new section for nightly builds
@@ -0,0 +1,195 @@ | |||
{ |
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.
Would be way more useful as a normalized JSON where each item in an array has id. ie. https://github.com/paularmstrong/normalizr
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 don't think this should be more complicated for no benefit. Leaving things like this is good.
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.
What do you need ids for? Isn't it better to access elements by name:
downloads["stable"]["Windows"]["qTox"] instead of downloads["stable"]["0"]["0"] ?
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.
Well it wouldn't be downloads["stable"]["0"]["0"], but my point is that it's more difficult to make a mistake with names and ids are more difficult to remember, because they don't mean anything. I don't understand why it would be more useful with ids. Maybe you could give an example?
|
||
jsonFile = open(DOWNLOADS_JSON_PATH) | ||
DOWNLOADS = json.load(jsonFile) | ||
jsonFile.close() |
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.
What if the file do not exists? Could we add a try/catch block here?
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.
If the file doesn't exist it will throw a Python error in the console and stop the site from building:
CRITICAL: IOError: [Errno 2] No such file or directory: u'downloads.json'
Makefile:28: recipe for target 'html' failed
make: *** [html] Error 1
We could have a try catch with our own messages for different kinds of exceptions, but is there an advantage?
themes/website/static/js/osdetect.js
Outdated
// Loop through all links and make buttons and info modals | ||
for (var i = 0; i < clients.length; i++) { | ||
for (var i = 0; i < clients.length; i++) |
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.
Rewrite it with length being cached in the loop please (best performances in case the clients list grow)
for (var i = 0, length = clients.length; i < length; i++) {
...
}
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.
Done.
themes/website/static/js/osdetect.js
Outdated
if (client.faicon) | ||
client.faicon = "fa fa-" + client.faicon; | ||
else | ||
client.faicon = ""; |
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.
Wouldn't the following be more concise?
var icon = client.faicon ? 'fa fa-' + client.faicon : '';
client.faicon = icon;
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 would, but won't it be less easy to read?
themes/website/static/js/osdetect.js
Outdated
</a>"; | ||
infoButton = document.createElement("A"); | ||
infoButton.className = "button large-button"; | ||
infoButton.style = "box-shadow:none;color:#368CCA"; |
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.
Applying style from JS makes it difficult to troubleshot issues with CSS later, make a class for that?
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.
Done.
@tox-user I prefer the alternative version of the button layout. |
@@ -0,0 +1,195 @@ | |||
{ |
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 don't think this should be more complicated for no benefit. Leaving things like this is good.
desc: false, | ||
dlLink: "http://wiki.tox.chat/binaries", | ||
dlLink: "http://wiki.tox.chat/binaries" |
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.
A nitpick comment, but you're not very consistent about where you have a comma after the last member in the struct and where you don't.
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 guess I only noticed that one for some reason. I'm working on a PR to remove all that hardcoded client information from JS, so it won't matter anymore.
icon: "icon-debian", | ||
desc: false, | ||
dlLink: "#gnulinux", | ||
}, |
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.
Another nitpick: in some places in this file, the braces between structs are done like so: }, {
without the endline after the first brace. It looks like the }, {
style was previously used, so you should probably stick to that.
buttonArea.appendChild(infoButton); | ||
|
||
buttonArea.innerHTML += "<br/>"; | ||
|
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.
Another nitpick: I don't see a lot of blank lines after the start or before the end of scopes in these files. What you've changed should probably stick to that.
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.
So I shouldn't separate things with new lines? I think it increases readability, but maybe it's just me.
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 don't think
buttonArea.appendChild (button);
if (infoButton)
buttonArea.appendChild(infoButton);
buttonArea.innerHTML += "<br/>";
}
is more readable than
buttonArea.appendChild (button);
if (infoButton)
buttonArea.appendChild(infoButton);
buttonArea.innerHTML += "<br/>";
}
The only place that I really noticed you using different whitespace from what was before is before or after braces.
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 agree that both those examples are as readable. I thought you meant something different.
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.
Also a good practice to put braces even for one line if. Allow faster refactoring/less issues with (bad) minification.
If you really wants it to be a one-line if, do this:
infoButton && buttonArea.appendChild(infoButton);
This works because the &&
operator always evaluate it's right instruction if the left instruction was true. Else it simply ignores the right instruction.
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.
Didn't know of that. Good to know!
Partially implements #120