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

Init file structure #4

Merged
merged 11 commits into from
Aug 24, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .babelrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"presets": ["react", "es2015"],
"plugins": [
"react-hot-loader/babel"
]
}
14 changes: 14 additions & 0 deletions .eslintrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
{
"parser": "babel-eslint",
"extends": "airbnb",
"globals": {
"__DEV__": true
},
"env": {
"browser": true,
"mocha": true
},
"rules": {
"react/jsx-filename-extension": 0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

According to kriasoft/react-starter-kit#52, it is recommended to use the .jsx file extension. What do you think?

Copy link
Owner Author

@esbb48 esbb48 Aug 21, 2016

Choose a reason for hiding this comment

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

but facebook's examples use .js.

and ... I like use .js
facebook/react#3582 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, feel free to keep it as it. I just want to let you know that jsx is the officially recommended file ext.

Copy link
Owner Author

Choose a reason for hiding this comment

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

okay :D

But officially? the discussion isn't react group right?

}
}
18 changes: 17 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
@@ -1 +1,17 @@
node_modules
# Logs
npm-debug.log*

# Dependency directory
node_modules

# Optional npm cache directory
.npm

# Optional REPL history
.node_repl_history

# Production Build
build/

.DS_Store
.idea/
3 changes: 3 additions & 0 deletions .stylelintrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"extends": "stylelint-config-standard",
}
17 changes: 17 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
@@ -1,2 +1,19 @@
# Kizuna
Friendship Game. For Vote, Rank, QA ...


### Run Project For Dev Mode

```
npm install
npm start
```

### Run lint

```
for all
npm run lint
or
npm run eslint
```
57 changes: 54 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
@@ -1,10 +1,16 @@
{
"name": "kizuna-platform",
"version": "1.0.0",
"version": "0.0.1",
"description": "Friendship Game. For Vote, Rank, QA ...",
"main": "index.js",
"scripts": {
"test": "echo \"Error: no test specified\" && exit 1"
"build": "npm run clean && webpack --config ./webpack.prod.config.js",
"clean": "rm -rf build",
"eslint": "./node_modules/.bin/eslint src test",
"lint": "npm run eslint && npm run stylelint",
"start": "./node_modules/.bin/webpack-dev-server --inline --devtool eval --progress --colors --hot --history-api-fallback --content-base dist",
"stylelint": "./node_modules/.bin/stylelint \"src/**/*.scss\" --syntax scss",
"test": "mocha test"
},
"repository": {
"type": "git",
Expand All @@ -22,5 +28,50 @@
"bugs": {
"url": "https://github.com/esbb48/Kizuna/issues"
},
"homepage": "https://github.com/esbb48/Kizuna#readme"
"homepage": "https://github.com/esbb48/Kizuna#readme",
"dependencies": {
"babel-polyfill": "^6.9.1",
"grommet": "^0.6.10",
"react": "^15.3.0",
"react-dom": "^15.3.0",
"react-hot-loader": "^3.0.0-beta.1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't react-hot-loader a devDependency?

Copy link
Owner Author

Choose a reason for hiding this comment

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

2016-08-21 7 19 59

Copy link
Collaborator

Choose a reason for hiding this comment

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

Strange, react-hot-loader is a devDependency in other repo. I'll check what's going on there.

"react-redux": "^4.4.5",
"react-router": "^2.6.1",
"react-router-redux": "^4.0.5",
"redux": "^3.5.2",
"redux-action": "^1.0.0",
"redux-saga": "^0.11.0"
},
"devDependencies": {
"autoprefixer": "^6.3.7",
"babel-core": "^6.11.4",
"babel-eslint": "^6.1.2",
"babel-loader": "^6.2.4",
"babel-plugin-transform-object-rest-spread": "^6.8.0",
"babel-preset-es2015": "^6.9.0",
"babel-preset-react": "^6.11.1",
"eslint": "^3.2.2",
"eslint-config-airbnb": "^10.0.0",
"eslint-plugin-import": "^1.12.0",
"eslint-plugin-jsx-a11y": "^2.0.1",
"eslint-plugin-react": "^6.0.0",
"extract-text-webpack-plugin": "^1.0.1",
"html-webpack-plugin": "^2.22.0",
"mocha": "^3.0.2",
"node-sass": "^3.8.0",
"postcss-loader": "^0.10.1",
"precss": "^1.4.0",
"sass-loader": "^4.0.0",
"should": "^11.1.0",
"stylelint": "^7.1.0",
"stylelint-config-standard": "^12.0.0",
"url-loader": "^0.5.7",
"webpack": "^1.13.1",
"webpack-dev-server": "^1.14.1",
"webpack-md5-hash": "0.0.5"
},
"engines": {
"node": "v6.2.0",
"npm": "3.8.9"
}
Copy link
Collaborator

@sharils sharils Aug 8, 2016

Choose a reason for hiding this comment

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

Do you think moving "stylelint" to .stylelintrc or stylelint.config.js according to stylelint/configuration.md at master · stylelint/stylelint is a good idea?

Copy link
Owner Author

Choose a reason for hiding this comment

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

done

}
Copy link
Collaborator

@sharils sharils Aug 8, 2016

Choose a reason for hiding this comment

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

Do you think adding engines according to package.json | npm Documentation would be a good idea?

Copy link
Owner Author

Choose a reason for hiding this comment

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

hmm?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi, I just fixed the link in my comment, can you check again? Thanks.

Copy link
Owner Author

Choose a reason for hiding this comment

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

thank you :D

Of course! It's good to work@@!!!!!!

29 changes: 29 additions & 0 deletions src/Auth/AuthContainer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import React, { PropTypes } from 'react';
// import { bindActionCreators } from 'redux';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dead code should be removed. What do you think?

Copy link
Owner Author

@esbb48 esbb48 Aug 21, 2016

Choose a reason for hiding this comment

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

may i keep it?

because it will be work on next issue.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, you can keep it. But it's generally a good idea to minimize each commit and changes set for each issue.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Okay I will notice for next time :)

import { connect } from 'react-redux';
import Header from 'grommet/components/Header';
Copy link
Collaborator

@sharils sharils Aug 6, 2016

Choose a reason for hiding this comment

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

This file is missing import { PropTypes } from 'react'; which is used in line 19.

Copy link
Owner Author

Choose a reason for hiding this comment

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

hmm ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Doh! This is a mistake. Please ignore this comment.

Copy link
Owner Author

Choose a reason for hiding this comment

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

okay


function AuthContainer({ children }) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

export function AuthContainer({ children }) { makes it easier to test than function AuthContainer({ children }) {. What do you think?

Copy link
Owner Author

Choose a reason for hiding this comment

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

hmmm why the method is more easier to test?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Without export, it's impossible to test the AuthContainer itself, you can only test the "connected" AuthContainer, did I explain myself?

Copy link
Owner Author

Choose a reason for hiding this comment

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

yeah .. sodaˇsga.

return (
<div>
<Header>My Header</Header>
{/* this will render the child routes */}
<div>
{children}
</div>
</div>
);
}

AuthContainer.propTypes = {
children: PropTypes.object,
};

function mapStateToProps(/* state */) {
return {};
}

function mapDispatchToProps(/* dispatch */) {
return {};
}
export default connect(mapStateToProps, mapDispatchToProps)(AuthContainer);
9 changes: 9 additions & 0 deletions src/Root/ErrorPage.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import React from 'react';

export default function ErrorPage() {
return (
<div>
ErrorPage
</div>
);
}
9 changes: 9 additions & 0 deletions src/Root/LandingPage.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import React from 'react';

export default function LandingPage() {
return (
<div>
LandingPage
</div>
);
}
19 changes: 19 additions & 0 deletions src/Root/Root.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import React, { PropTypes } from 'react';
import { Provider } from 'react-redux';
import { Router } from 'react-router';

import routes from './routes';


export default function Root({ store, history }) {
return (
<Provider store={store}>
<Router history={history} routes={routes} />
</Provider>
);
}

Root.propTypes = {
history: PropTypes.object.isRequired,
store: PropTypes.object.isRequired,
};
4 changes: 4 additions & 0 deletions src/Root/index.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
body {
height: 100%;
width: 100%;
}
8 changes: 8 additions & 0 deletions src/Root/rootReducer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { combineReducers } from 'redux';
import { routerReducer as routing } from 'react-router-redux';

const rootReducer = combineReducers({
routing,
});

export default rootReducer;
6 changes: 6 additions & 0 deletions src/Root/rootSaga.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// import { fork } from 'redux-saga/effects';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dead code should be removed. What do you think?

Copy link
Owner Author

Choose a reason for hiding this comment

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


export default function* rootSaga() {
yield [
];
}
12 changes: 12 additions & 0 deletions src/Root/routes.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import React from 'react';
import { Route } from 'react-router';
import AuthContainer from '../Auth/AuthContainer';
import ErrorPage from './ErrorPage';
import LandingPage from './LandingPage';

export default (
<Route path="/" component={AuthContainer}>
<Route path="home" component={LandingPage} />
<Route path="*" component={ErrorPage} />
</Route>
);
14 changes: 14 additions & 0 deletions src/index.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<!DOCTYPE html>
<html>
<head>
<title>Kizuna</title>
<!-- Setting the right viewport -->
<meta charset="utf-8" />
<meta name="viewport" content="width=device-width, initial-scale=1.0, maximum-scale=1.0, user-scalable=no" />
<link rel='stylesheet prefetch' href='http://grommet.io/assets/latest/css/grommet.min.css'>
</head>
<body>
<div id="root">
</div>
</body>
</html>
40 changes: 40 additions & 0 deletions src/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/* eslint global-require: 0 */
Copy link
Collaborator

@sharils sharils Aug 8, 2016

Choose a reason for hiding this comment

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

It seems that this line isn't used. Should it be removed?

Copy link
Owner Author

Choose a reason for hiding this comment

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

2016-08-21 9 00 28

Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting again, in my other code base, it doesn't complain about use of require. I'll investigate into that.

Copy link
Owner Author

Choose a reason for hiding this comment

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

hmm. If I use .jsx, it will pass the lint.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How weird!!

import 'babel-polyfill';
import React from 'react';
import { render } from 'react-dom';
import { browserHistory as history } from 'react-router';
import { syncHistoryWithStore } from 'react-router-redux';
import { AppContainer } from 'react-hot-loader';

import rootSaga from './Root/rootSaga';

import configureStore from './store/configureStore';
import Root from './Root/Root';

const store = configureStore({ history });

const syncedHistory = syncHistoryWithStore(history, store);

const rootElement = document.getElementById('root');

store.runSaga(rootSaga);

render((
<AppContainer>
<Root store={store} history={syncedHistory} />
</AppContainer>
), rootElement
);

if (module.hot) {
module.hot.accept('./Root/Root', () => {
const NextApp = require('./Root/Root').default;

render((
<AppContainer>
<NextApp store={store} history={syncedHistory} />
</AppContainer>
), rootElement
);
});
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there should be a way to merge duplicated code in this file, what do you think?

Copy link
Owner Author

Choose a reason for hiding this comment

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

but it's a sample code from document @@.

Should I change ?

如果做了感覺很繞

Copy link
Collaborator

@sharils sharils Aug 21, 2016

Choose a reason for hiding this comment

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

If you think it's now the right time to do it or the reason can't convince you. Feel free to ignore it. :)

Copy link
Owner Author

@esbb48 esbb48 Aug 21, 2016

Choose a reason for hiding this comment

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

I don't have a good idea now>
but I open the issue to resolve this in the feature = = +

#5 duplicated code in src/index.js

22 changes: 22 additions & 0 deletions src/store/configureStore.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { compose, createStore, applyMiddleware } from 'redux';
import { routerMiddleware } from 'react-router-redux';
import createSagaMiddleware, { END } from 'redux-saga';
import rootReducer from '../Root/rootReducer';
// import rootSaga from '../Root/rootSaga';

export default function configureStore({ initialState = {}, history }) {
const route = routerMiddleware(history);
const sagaMiddleware = createSagaMiddleware();
const store = createStore(
rootReducer,
initialState,
compose(
applyMiddleware(route, sagaMiddleware)
)
);

store.runSaga = sagaMiddleware.run;
store.close = () => store.dispatch(END);

return store;
}
3 changes: 3 additions & 0 deletions test/mocha.opts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
--compilers js:babel-core/register
--require should
--require babel-polyfill
56 changes: 56 additions & 0 deletions webpack.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
const HtmlWebpackPlugin = require('html-webpack-plugin');
const PORT = process.env.PORT || 3000;

module.exports = {
devtool: 'cheap-module-eval-source-map',
entry: [
'react-hot-loader/patch',
`webpack-dev-server/client?http://localhost:${PORT}`,
'webpack/hot/only-dev-server',
'./src/index.js',
],
module: {
loaders: [
{
test: /\.js(x?)$/,
exclude: /node_modules/,
loaders: ['babel'],
}, {
test: /\.(png|jpg|jpeg|gif|svg|woff|woff2|eot)$/,
loader: 'url',
query: {
name: '[path][name].[ext]?[hash]',
limit: 500,
},
}, {
test: /\.scss$/,
loaders: [
'style?sourceMap',
'css?modules&importLoaders=1&localIdentName=[name]__[local]___[hash:base64:5]',
'postcss',
'sass?sourceMap',
],
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think it'd be a good idea to always use scss and leave out css?

Copy link
Owner Author

Choose a reason for hiding this comment

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

hmm?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you elaborate? @@?

Copy link
Owner Author

Choose a reason for hiding this comment

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

ahh I understant !

],
},
resolve: {
extensions: ['', '.js', '.jsx'],
},
output: {
filename: 'bundle.js',
path: `${__dirname}/dist`,
publicPath: '/',
},
devServer: {
port: PORT,
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think it'd be a good idea to move all CLI switches in npm start to be under this devServer property?

Copy link
Owner Author

Choose a reason for hiding this comment

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

hmmm how about the port ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you elaborate?

Copy link
Owner Author

Choose a reason for hiding this comment

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

ahh I got it !

postcss: [
require('autoprefixer'),
require('precss'),
],
plugins: [
new HtmlWebpackPlugin({
template: './src/index.html',
}),
],
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Through out this file, some loaders are used with the -loader suffix while some aren't. Do you think it's a good idea to make a decision to always use the -loader suffix or never use the -loader suffix?

Copy link
Owner Author

@esbb48 esbb48 Aug 21, 2016

Choose a reason for hiding this comment

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

okay.

remove -loader

Loading