Skip to content

Conversation

@gnestor
Copy link

@gnestor gnestor commented Mar 2, 2018

This moves react and react-dom from dependencies to peerDependencies and devDependencies. This prevents multiple copies of react from being loaded by consumer apps.

The same needs to be done for plotly-icons: plotly/plotly-icons#14

When that's merged, we can change the:

"plotly-icons": "github:gnestor/plotly-icons#package.json",

line to:

"plotly-icons": "latest",

@nicolaskruchten
Copy link
Contributor

This makes sense to me. I didn't realize you could have modules in both peerDeps and devDeps.

@nicolaskruchten
Copy link
Contributor

@VeraZab let's get this and the icons one merged in before the next release

@VeraZab
Copy link
Contributor

VeraZab commented Mar 3, 2018

thanks @gnestor I'll make a release for plotly-icons now and then we can change this in your pr and then merge

@VeraZab
Copy link
Contributor

VeraZab commented Mar 3, 2018

ok, plotly-icons 1.1.5 is out, we can make changes here for it, then merge this :) thanks!

@gnestor
Copy link
Author

gnestor commented Mar 4, 2018

@VeraZab Ok, I updated this branch 👍

@nicolaskruchten
Copy link
Contributor

💃 once this passes CI.

@nicolaskruchten
Copy link
Contributor

Ah, perhaps CI doesn't run on external repos? I'm OK to merge this @VeraZab

@VeraZab
Copy link
Contributor

VeraZab commented Mar 5, 2018

yes @nicolaskruchten ok to merge. there's no conflicts in package.json right?

@gnestor
Copy link
Author

gnestor commented Mar 5, 2018

Github should warn you if there are merge conflicts. It looks mergeable to me. I rebase this to master yesterday 👍

@nicolaskruchten
Copy link
Contributor

This is what the UI looks like in github for us now:

image

@gnestor
Copy link
Author

gnestor commented Mar 5, 2018

Ok, check now 👍

@nicolaskruchten
Copy link
Contributor

Hehe thanks, but in the meantime I submitted the same changes in a separate PR so they could hit master faster. There's something about our branch-protection setup which is making things hard for cross-repo PRs I think.

@VeraZab VeraZab reopened this Mar 5, 2018
@VeraZab VeraZab closed this Mar 5, 2018
@gnestor
Copy link
Author

gnestor commented Mar 5, 2018

Ok, in the future I'll try to submit PRs from branches on this repo (vs. a fork) 👍

@gnestor gnestor deleted the dependencies branch March 5, 2018 19:21
@nicolaskruchten
Copy link
Contributor

Thanks, and sorry, I'm not sure why this happens...

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.

3 participants