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

Lerna #7

Merged
merged 31 commits into from Sep 4, 2017
Merged

Lerna #7

merged 31 commits into from Sep 4, 2017

Conversation

ansumanshah
Copy link
Owner

@ansumanshah ansumanshah commented Sep 2, 2017

WIP #5
Merging the vscode plugin to add the autocomplete
@paulmolluzzo
How does the publish to vscode work can we change the name of the extension?
Any suggestions from your side?

@paulmolluzzo
Copy link
Collaborator

paulmolluzzo commented Sep 3, 2017

Thanks for doing this @ansumanshah! 🙌 It's definitely a great start to merging the two packages, and then from there it'll be much easier to maintain. Nice! This lerna pack is super cool!

There are a few issues with this PR regarding the VS Code version, and also a few places we can make improvements. A big plus to this PR is that we can start sharing code, so that'd be great to get started.

Since I can't push on this PR I'll just list them:

  • I'm not able build the VS Code version without symlinking/adding .babelrc to the project or changing the build script to: "build": "babel --no-babelrc src -d dist".
  • ~/packages/vscode-css-in-js/dist/convert.js and ~/packages/atom-css-in-js/lib/convertLines.js should be functionally identical. When I moved it to the other repo I just cleaned up the code style. Compare them yourself, and then let's try to use the same file in a common location for both versions, like in a common lib directory or something.
    ** If this file is moved then the import in ~/packages/vscode-css-in-js/src/extension.js needs to be updated too.
  • We should have a README in the root. For now something simple should be OK, but without one the GH page will look weird. 😅

That's all I'm seeing at the moment. So awesome @ansumanshah! 👏 👏 👏

Edit: if there's a way to update the PR please let me know how and I can get these items done.

@ansumanshah
Copy link
Owner Author

I am extremely sorry just added you as a collaborator. This looks like a great start, to begin with. Let's update this PR only with all the required changes

@paulmolluzzo
Copy link
Collaborator

@ansumanshah I pushed some changes last night to fix the merge conflict and address the items I listed. Correcting the merge conflict meant updating the atom version, so please take a close look.

If we're OK to merge this, I'll work on the autocomplete integration after it's merged.

Also, just thinking out loud, I'll have to update the VS Code package.json and publish again to show the new repo location and stuff in the marketplace. I would like to keep using the same package in the marketplace so current users just get updates. I'll publish those changes with the autocomplete update since it seems like the best time.

I am extremely sorry just added you as a collaborator.

LOL, you don't have to apologize! Thanks for adding me! 🙌

@ansumanshah ansumanshah merged commit 9b1aedc into master Sep 4, 2017
@ansumanshah ansumanshah deleted the lerna branch September 4, 2017 20:43
@ansumanshah
Copy link
Owner Author

merged this to master, with some changes created a package for the helper functions (that will maybe contain the upcoming codemods too)

I believe there should be a way to change the name of the package, and the users will still get the update with the new name convert-css-in-js does not sound that it should contain autocomplete too, just a thought.

@paulmolluzzo
Copy link
Collaborator

I believe there should be a way to change the name of the package, and the users will still get the update with the new name convert-css-in-js does not sound that it should contain autocomplete too, just a thought.

I agree. I'll make that change with the others I mentioned. 👍

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.

None yet

2 participants