-
Notifications
You must be signed in to change notification settings - Fork 78
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
fix: Only export components when publishing #177
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking merge until permissions are fixed for fundamental-bot
Also I'm up for a name thats different from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few things:
- I am still seeing the
documentation
directory show up indist
. Should it be? - If we are going to isolate the playground from the other
src
directories, I would prefer aplayground
directory at the root rather than a_playground
directory withinsrc
. Just my opinion. - In the
src/index.js
file, it is importingreact
andreact-dom
, but I don't believe it needs to be. - In the
src/index.js
file, I am wondering if this file should be refactored so each line is the import AND export to make it more readable. Example:
export {Alert} from '../src/Alert/Alert';
Hey @greg-a-smith! |
src/index.js
Outdated
|
||
ReactDOM.render(<App />, document.getElementById('root')); | ||
registerServiceWorker(); | ||
export { ActionBar } from '../src/ActionBar/ActionBar'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all of these should be rewritten to NOT include the src
directory as I am still seeing the src
directory in the path in the dist
directory.
So:
export { ActionBar } from '../src/ActionBar/ActionBar';
should become:
export { ActionBar } from './ActionBar/ActionBar';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. 🚢
I spoke with @jeffredodd, who is out sick, about taking over this PR to get it merged. |
* Initial change to only export library * Remove documentation folder from Components * Remove react and react-dom from component exports * Updating index.js file to contain individual exports * Quick fix to src src-ing * Changed the jest coverage ignore path of the playground components
Hey everyone, the goal of this PR is to only publish our components to
npm
and to leave the tests, component documentation, and other CreateReactApp leftovers behind. This will reduce the payload we send to consumers and simply our outputted package while leaving the structure mostly intact to allow for simple contributions and also resolves #105.I have performed the following...
_playground
_playground
folderindex.js
file to no longer have documentation site componentsRoutes.js
to point to existing components correctlyFeel free to pull this down and test the changes i've made and let me know what you think. It might help to poke around on
master
'sdist
folder first to see what was omitted in this branch.This branch also should not go in until publishing on
master
is fixed.