Skip to content
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

Two pages json.html and objects.html #81

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Two pages json.html and objects.html #81

wants to merge 2 commits into from

Conversation

hammadirshad
Copy link

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

@hammadirshad
Copy link
Author

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

@adrai
Copy link
Owner

adrai commented May 17, 2016

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

@hammadirshad
Copy link
Author

No, I just rebuild the project

@adrai
Copy link
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
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
Owner

adrai commented May 17, 2016

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

@hammadirshad
Copy link
Author

Ok I will rebase release folder


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

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.


var data = JSON.parse(code);

var symbols=data.symbols;

Choose a reason for hiding this comment

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

Spacing/formatting


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

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
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.

var direction=directions[key];
directionsArray[direction.key+"_"+direction.nextKey+"_"+direction.next]=direction;
}

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.



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

Choose a reason for hiding this comment

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

Remove dead code

}
});

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

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?

var data = {
symbols: {
st: {
key: 'st',

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.

None yet

4 participants