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

Add support for export default #55

Merged
merged 1 commit into from
Apr 9, 2019
Merged

Add support for export default #55

merged 1 commit into from
Apr 9, 2019

Conversation

jb-san
Copy link

@jb-san jb-san commented Nov 14, 2018

this adds the ability to do export { default } from ... and export { default as Icon } from ...
this is my first time doing anything with babel, so criticism is welcome..

Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

I'll rereview after the unrelated style changes are rebased out.

.eslintrc Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
test/sanity.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This looks great overall, thanks! I’m a bit hesitant about the camelize/capitalize approach, but i don’t have a better suggestion.

I wonder if it would be better to leave it anonymous in that case, but set the displayName property to directly match the filename?

src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/capitalize.js Outdated Show resolved Hide resolved
@jb-san jb-san force-pushed the export branch 2 times, most recently from ff409ef to fcda0e3 Compare November 25, 2018 12:57
Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Seems like only a few more small tweaks.

It’d still be nice to combine the “with defaults” code more into the buildSvg code, since there’s so much repeated.

.npmrc Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/index.js Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
@jb-san
Copy link
Author

jb-san commented Nov 26, 2018

anything else that needs fixing ?

@jb-san
Copy link
Author

jb-san commented Jan 24, 2019

should i just close this ? im using it in a project and everything is working, but i can just make a private repo and use that

@ljharb
Copy link
Collaborator

ljharb commented Feb 6, 2019

@jb-san please don't; sorry for the delays but my time is limited. I'll plan to merge and release this as soon as i can.

@ljharb ljharb merged commit 4ed020e into airbnb:master Apr 9, 2019
ljharb added a commit that referenced this pull request Apr 10, 2019
 - [New] Add support for export default from (#55)
 - [meta] add MIT license file (#60)
 - [Deps] update `resolve`
 - [Dev Deps] update `@babel/cli`, `@babel/node`, `babel-preset-airbnb`, `eslint`, `eslint-plugin-import`, `eslint-plugin-jsx-a11y`, `eslint-plugin-react`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants