Skip to content

Two pages json.html and objects.html#81

Open
hammadirshad wants to merge 2 commits into
adrai:masterfrom
hammadirshad:master
Open

Two pages json.html and objects.html#81
hammadirshad wants to merge 2 commits into
adrai:masterfrom
hammadirshad:master

Conversation

@hammadirshad
Copy link
Copy Markdown

JSON support added in lib so language like java and C# easily create flowcharts

@hammadirshad
Copy link
Copy Markdown
Author

JSON support added in lib so language like java and C# easily create flowcharts

@adrai
Copy link
Copy Markdown
Owner

adrai commented May 17, 2016

can you delete all unneeded files? i.e release/* iml?, etc...

@hammadirshad
Copy link
Copy Markdown
Author

No, I just rebuild the project

@adrai
Copy link
Copy Markdown
Owner

adrai commented May 17, 2016

ok, but you could revert them... because I will do the build when releasing a new version...
but can you provide a bit of doc in the readme?

@hammadirshad
Copy link
Copy Markdown
Author

Do you have any develop branch I will make marge request with that branch. And for this reject this pull requests

@adrai
Copy link
Copy Markdown
Owner

adrai commented May 17, 2016

no.... but if you want you can rebase your changes...

@hammadirshad
Copy link
Copy Markdown
Author

Ok I will rebase release folder

Comment thread example/json.html

window.onload = function () {
var btn = document.getElementById("run"),
cd = document.getElementById("code"),
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're already putting these on their own lines just declare the "var" for each. Makes your intentions a little clearer.

Comment thread example/json.html

var data = JSON.parse(code);

var symbols=data.symbols;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spacing/formatting

Comment thread example/json.html

var symbolsArray ={};
for(var key in symbols)
{
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we going with this C style curly braces?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bad idea in JavaScript. Every newLine triggers a “Should a ; be auto inserted. In compiled languages like C and Java the position of the { is a question of taste (and a source of religious wars second only to tabs vs spaces). In JavaScript it matters. Keep them at line end.

Comment thread example/json.html
var direction=directions[key];
directionsArray[direction.key+"_"+direction.nextKey+"_"+direction.next]=direction;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make a note of these extra spaces and formatting between expressions.

Comment thread example/json.html


chart.drawSVG('canvas', {
// 'x': 30,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove dead code

Comment thread example/json.html
}
});

$('[id^=sub1]').click(function(){
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What exactly is this supposed to do?

Comment thread example/objects.html
var data = {
symbols: {
st: {
key: 'st',
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems a bit like a maintenance nightmare to have a property in all these objects with the key name. You can always get the key name with builtin functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants