Add generating downloads from JSON #187

tox-user · 2018-01-22T05:13:32Z

Partially implements #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

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

SkyzohKey · 2018-01-23T16:40:17Z

downloads.json

@@ -0,0 +1,195 @@
+{


Would be way more useful as a normalized JSON where each item in an array has id. ie. https://github.com/paularmstrong/normalizr

I don't think this should be more complicated for no benefit. Leaving things like this is good.

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"] ?

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?

SkyzohKey · 2018-01-23T16:41:07Z

pelicanconf.py

+
+jsonFile = open(DOWNLOADS_JSON_PATH)
+DOWNLOADS = json.load(jsonFile)
+jsonFile.close()


What if the file do not exists? Could we add a try/catch block here?

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?

SkyzohKey · 2018-01-23T16:43:07Z

themes/website/static/js/osdetect.js

 // 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++) 


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++) { ... }

SkyzohKey · 2018-01-23T16:44:33Z

themes/website/static/js/osdetect.js

+	if (client.faicon)
+		client.faicon = "fa fa-" + client.faicon;
+	else
+		client.faicon = "";


Wouldn't the following be more concise?

var icon = client.faicon ? 'fa fa-' + client.faicon : ''; client.faicon = icon;

It would, but won't it be less easy to read?

SkyzohKey · 2018-01-23T16:45:06Z

themes/website/static/js/osdetect.js

-		</a>";
+		infoButton = document.createElement("A");
+		infoButton.className = "button large-button";
+		infoButton.style = "box-shadow:none;color:#368CCA";


Applying style from JS makes it difficult to troubleshot issues with CSS later, make a class for that?

tox-user · 2018-01-23T23:58:47Z

I reverted download buttons to how they used to be before - one in each line. At first I thought it would be a good idea to make them like on the screenshot above, but I'm not sure anymore. What does everyone think?

Current

Alternative

Current

Alternative

robinlinden · 2018-01-25T21:11:08Z

@tox-user I prefer the alternative version of the button layout.

robinlinden · 2018-01-23T16:48:28Z

downloads.json

@@ -0,0 +1,195 @@
+{


I don't think this should be more complicated for no benefit. Leaving things like this is good.

robinlinden · 2018-01-25T21:20:48Z

themes/website/static/js/osdetect.js

 	desc: false,
-	dlLink: "http://wiki.tox.chat/binaries",
+	dlLink: "http://wiki.tox.chat/binaries"


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.

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.

robinlinden · 2018-01-25T21:24:19Z

themes/website/static/js/osdetect.js

+		icon: "icon-debian",
+		desc: false,
+		dlLink: "#gnulinux",
+	},


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.

robinlinden · 2018-01-25T21:26:30Z

themes/website/static/js/osdetect.js

+		buttonArea.appendChild(infoButton);
+
+	buttonArea.innerHTML += "<br/>";
+


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.

So I shouldn't separate things with new lines? I think it increases readability, but maybe it's just me.

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.

I agree that both those examples are as readable. I thought you meant something different.

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.

Didn't know of that. Good to know!

Add generating downloads from JSON

6f2e25e

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

SkyzohKey suggested changes Jan 23, 2018

View reviewed changes

Fix JavaScript and style

2087281

robinlinden reviewed Jan 25, 2018

View reviewed changes

nurupo force-pushed the master branch from 893a416 to 9a9105b Compare April 28, 2018 16:34

tox-user closed this Jun 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add generating downloads from JSON #187

Add generating downloads from JSON #187

tox-user commented Jan 22, 2018

SkyzohKey Jan 23, 2018

robinlinden Jan 23, 2018

tox-user Jan 23, 2018

tox-user Jan 23, 2018

SkyzohKey Jan 23, 2018

tox-user Jan 23, 2018

SkyzohKey Jan 23, 2018

tox-user Jan 23, 2018

SkyzohKey Jan 23, 2018

tox-user Jan 23, 2018

SkyzohKey Jan 23, 2018

tox-user Jan 23, 2018

tox-user commented Jan 23, 2018

robinlinden commented Jan 25, 2018

robinlinden Jan 23, 2018

robinlinden Jan 25, 2018

tox-user Jan 25, 2018

robinlinden Jan 25, 2018

robinlinden Jan 25, 2018

tox-user Jan 25, 2018

robinlinden Jan 25, 2018

tox-user Jan 25, 2018

SkyzohKey Jan 31, 2018 •

edited

Loading

tox-user Feb 1, 2018

		buttonArea.appendChild(infoButton);

		buttonArea.innerHTML += "<br/>";

Add generating downloads from JSON #187

Add generating downloads from JSON #187

Conversation

tox-user commented Jan 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tox-user commented Jan 23, 2018

robinlinden commented Jan 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SkyzohKey Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SkyzohKey Jan 31, 2018 •

edited

Loading