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

Conversation

esbb48
Copy link
Owner

@esbb48 esbb48 commented Aug 4, 2016

Ticket :: #2 Initial the file structure.

@@ -0,0 +1,6 @@
{
"presets": ["react", "es2015", "stage-0"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

stage-0

What ES7 features do you want to use? I think it's better to name those and include those.

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.

Not pretty sure for now.
And .. I can't understand 'I think it's better to name those and include those.'.
It's mean that I only get what I need?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's mean that I only get what I need?

Yes, let's what I mean. Since you don't know what you want to use now. Do you think it's a good idea to add when you want it?

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

haha .. i find a typo ... from me (know / new 囧rz)

@sharils
Copy link
Collaborator

sharils commented Aug 8, 2016

I'm curious. Do you order imports in some way? They look like they have an order. Could you tell me? Thanks.

@@ -0,0 +1,6 @@
// import { fork } from 'redux-saga/effects';

export default function* root() {
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.

I just found that there is import rootSaga from './Root/rootSaga'; in the src/index.js file. Do you think it'd be a good idea to rename root to rootSaga?

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

})
],

};
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

@sharils
Copy link
Collaborator

sharils commented Aug 10, 2016

In webpack.config.js, dist is used while in webpack.prod.config.js, build is used. Do you think it'd be a good idea to use either dist or build across the whole code base?

contentBase: './dist',
port: PORT,
hot: true,
},
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 !

@sharils
Copy link
Collaborator

sharils commented Aug 10, 2016

Hi @esbb48,

I just finished code review. Please let me know if you have any question.

Best,
@sharils

P.s. I skipped code review for webpack.prod.config.js.

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.

Do you want to fix this file?

Copy link
Owner Author

Choose a reason for hiding this comment

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

yes, I fixed it

@sharils
Copy link
Collaborator

sharils commented Aug 21, 2016

Hi @esbb48 , I just finished commenting. Please review. Thanks. I saw a lot of done. But I didn't see any follow up commits on this branch. Do you forgot to push them? Thanks.

@esbb48
Copy link
Owner Author

esbb48 commented Aug 21, 2016

HI @sharils,

Because I push wrong branch ....
Please try again.

"babel-plugin-transform-object-rest-spread": "^6.8.0",
"babel-preset-es2015": "^6.9.0",
"babel-preset-react": "^6.11.1",
"babel-preset-stage-0": "^6.5.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you forget to remove "babel-preset-stage-0": "^6.5.0" ? :)

Copy link
Owner Author

Choose a reason for hiding this comment

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

Finish !

@sharils
Copy link
Collaborator

sharils commented Aug 24, 2016

LGTM except for my last comment. Cheers! :)

@esbb48
Copy link
Owner Author

esbb48 commented Aug 24, 2016

@sharils sharils merged commit 8862844 into development Aug 24, 2016
@esbb48 esbb48 deleted the feature/2_20160804_ali_initialFileStructure branch December 6, 2016 15:00
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.

2 participants