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

Full refactor #1

Merged
merged 41 commits into from
Jan 4, 2017
Merged

Full refactor #1

merged 41 commits into from
Jan 4, 2017

Conversation

cef62
Copy link
Collaborator

@cef62 cef62 commented Jan 3, 2017

Completely rewritten implementation of the library to benefit of the new react-styleguidist API to configure the undelying express server.

Also the setup is reduced to a single action: just process the project styleguide.config.js module to snapguidist:

const snapguidist = require('snapguidist')

module.exports = snapguidist({ [...] })

cef62 added 30 commits December 31, 2016 14:10
Copy link
Collaborator

@MicheleBertoli MicheleBertoli left a comment

Choose a reason for hiding this comment

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

Awesome job, thanks!
Just a few comment.

@@ -1,3 +1,3 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need this file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree the source doesn't need it but we have to keep it to run our tests ;)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Aha, we need it for the tests. Thanks @cef62 :)
Just remove stage-0.

@@ -1,2 +1,6 @@
dist
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be:

.opt-out
.opt-in
.snapguidist
node_modules
npm-debug.log

"react-styleguidist": "^4.4.1",
"webpack": "^1.14.0"
"babel-core": "6.20.0",
"babel-loader": "6.2.10",
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need babel-core and babel-loader.

const path = require('path')
const snapguidist = require(path.join(__dirname, '..', 'src', 'index'))


Copy link
Collaborator

Choose a reason for hiding this comment

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

Unnecessary new line.

"babel-jest": "18.0.0",
"babel-preset-es2015": "6.18.0",
"babel-preset-react": "6.16.0",
"babel-preset-stage-0": "6.16.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need babel-preset-stage-0.

},
"peerDependencies": {
"react-styleguidist": "^4.4.1"
"react": ">15",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be:

"react": ">=15",

include: srcFolder,
loader: 'babel',
query: {
presets: [
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be:

presets: [
  'es2015',
  'react',
],

}
)

// TODO: should be avoided to add style.css as last item in `entry`?
Copy link
Collaborator

Choose a reason for hiding this comment

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

// TODO: please remove TODOS :)



module.exports = snapguidist({
title: 'Snapguidist Development Example',
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd try to keep the name consistent, what do you think about:

[dev] Snapguidist Styleguide

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like that ;)

@@ -16,8 +16,8 @@ class SnapguidistPlaygroundRenderer extends Component {
}

SnapguidistPlaygroundRenderer.propTypes = {
code: PropTypes.string.isRequired,
evalInContext: PropTypes.func.isRequired,
// code: PropTypes.string.isRequired,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure about this comments, should we remove the propTypes?

@MicheleBertoli MicheleBertoli merged commit af8b29f into master Jan 4, 2017
@MicheleBertoli MicheleBertoli deleted the huge-refactor branch January 4, 2017 09:35
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