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

Reactification #24

Merged
merged 34 commits into from Nov 10, 2015
Merged

Reactification #24

merged 34 commits into from Nov 10, 2015

Conversation

@peric
Copy link
Owner

peric commented May 10, 2015

No description provided.

index.html Outdated
<link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.4/css/bootstrap.min.css">
<link rel="stylesheet" type="text/css" href="css/style.css" />
<script src="https://fb.me/react-0.13.2.js"></script>
<script src="https://fb.me/JSXTransformer-0.13.2.js"></script>

This comment has been minimized.

Copy link
@zigomir

zigomir May 10, 2015

Contributor

you wan't to have build process done locally and not running transformer on other people's machines ;)

index.html Outdated
<link rel="stylesheet" type="text/css" href="css/style.css" />
<script src="https://fb.me/react-0.13.2.js"></script>
<script src="https://fb.me/JSXTransformer-0.13.2.js"></script>
<script src="https://code.jquery.com/jquery-2.1.3.min.js"></script>

This comment has been minimized.

Copy link
@zigomir

zigomir May 10, 2015

Contributor

Not sure, but do you need jquery? I see you do. Since you have React now, I think we can solve problem without it...we'll talk :)

js/app.js Outdated
@@ -0,0 +1,548 @@
var OUTPUT_MYSQL = 'MySQL',

This comment has been minimized.

Copy link
@zigomir

zigomir May 10, 2015

Contributor

This file can easily be split to few smaller files.

js/app.js Outdated
this.forceUpdate();

// TODO: do I need this? How forceUpdate does updates based on references?
// this.setState({type: data});

This comment has been minimized.

Copy link
@zigomir

zigomir May 10, 2015

Contributor

Use setState wherever you can. forceUpdate should be your last exit. setState will trigger re-render of component and this is usually what you want.

js/app.js Outdated
longName={setting.longName}
checked={setting.checked}
onChange={self.toggleCheck}
disabled={setting.disabled} />

This comment has been minimized.

Copy link
@zigomir

zigomir May 10, 2015

Contributor

props for using lots of props 👍 :)

js/app.js Outdated
<Column
key={index + column.name}
objectKey={key}
data={self.state.columns}

This comment has been minimized.

Copy link
@zigomir

zigomir May 10, 2015

Contributor

data is really generic name. I believe you can think of something more specific ;)

js/app.js Outdated
</div>
);
}
});

This comment has been minimized.

Copy link
@zigomir

zigomir May 10, 2015

Contributor

Liking small components like Setting, OutputType and Column you did here 👍

js/app.js Outdated
" PRIMARY KEY (`countryCode`,`languages`)\n" +
") ENGINE=MyISAM DEFAULT CHARSET=utf8;\n\n" +
"{2}\n" +
"{3}";

This comment has been minimized.

Copy link
@zigomir

zigomir May 10, 2015

Contributor

ohnoes, this hurts. We're gonna convert this project to ES6 so you can use template strings

js/main.js Outdated
this.select();

// TODO: copy to clipboard
// https://github.com/zeroclipboard/zeroclipboard

This comment has been minimized.

Copy link
@zigomir

zigomir May 10, 2015

Contributor

👍

@peric
Copy link
Owner Author

peric commented May 10, 2015

@zigomir, thanks a lot for that useful review and commit!

@zigomir zigomir mentioned this pull request Jul 16, 2015
peric added a commit that referenced this pull request Nov 10, 2015
@peric peric merged commit 209b798 into master Nov 10, 2015
@peric peric deleted the reactify branch Nov 10, 2015
@zigomir
Copy link
Contributor

zigomir commented Nov 10, 2015

hhh 👍

@peric peric mentioned this pull request Nov 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.