feat: use react-loading-spinner #33
Conversation
remove dependency on private icons
Codecov Report
@@ Coverage Diff @@
## master #33 +/- ##
=======================================
Coverage 63.71% 63.71%
=======================================
Files 15 15
Lines 565 565
Branches 146 146
=======================================
Hits 360 360
Misses 205 205
Continue to review full report at Codecov.
|
felixfbecker
left a comment
felixfbecker
left a comment
There was a problem hiding this comment.
We're gonna need this in at least 3 repos and I want to make sure they stay consistent - could we put this in an npm package?
|
Yes we could, but my pragmatic observation is that the other two repos depend on this repo, so this saved me the effort of setting up a new repo and npm package. We can extract it to its own npm package later if we want, but if you insist, I can do it now. |
|
Setting up a new repo and npm package should only take a few minutes, so please do :) |
|
I am sure it would only take you a few minutes. Do you see how much boilerplate is in this repo? How much of that would you expect in a loader npm package? |
|
I usually just copy-paste another repo. Would it help to have an official "template" repo that can be duplicated for new packages? Or an interactive generator? I would like everyone on the team to feel comfortable with doing this - boilerplate (that can be automated) should not be the thing holding us back from exercising proper separation of concerns and code sharing. I've seen a lot of issues in the browser extension caused by out-of-date copy-pasted code so this is important to me |
Yes, guidance here would have been helpful, especially moving forward. I copied codeintellify because I knew that was the latest one that was created, but in general it will be hard for an arbitrary dev on the team to know which is best to copy. For the record this took me about an hour of extra work (this is the first package I have published so I had to look up some boring stuff). I wouldn't say that I am happy about publishing a package that contains more boilerplate than code, but it is done now. I am sure you will find things that could be improved: https://github.com/sourcegraph/react-loading-spinner |
src/HoverOverlay.scss
Outdated
| } | ||
| } | ||
|
|
||
| @import 'node_modules/@sourcegraph/react-loading-spinner/lib/LoadingSpinner.css'; |
There was a problem hiding this comment.
Put at the top?
Also, shouldnt this be ../node_modules?
There was a problem hiding this comment.
I was copying the style here (both putting it at the bottom and the import path). I can move this to the top if it is preferred.
I tested with npm run build; npm pack; npm --prefix ../browser-extension i ./sourcegraph-codeintellify-0.0.0-DEVELOPMENT.tgz and both node_modules/... and ../node_modules/... seem to work, so not sure which is preferred.
There was a problem hiding this comment.
Top is preferred, but in the example you linked it has to be at the bottom because it has to come after Bootstrap (to override things), but before importing Bootstrap its configuration variables have to be defined.
The import still worked because our webpack config falls back to resolving relative to the root too, but I wouldn't want to rely on that in this package.
|
🎉 This PR is included in version 3.6.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
remove dependency on private icons
part of https://github.com/sourcegraph/sourcegraph/issues/12633
demo (top is new, bottom is old):
