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

Fixed the url for Broken logo #126

Merged
merged 8 commits into from
Aug 30, 2017
Merged

Fixed the url for Broken logo #126

merged 8 commits into from
Aug 30, 2017

Conversation

cg-cnu
Copy link
Contributor

@cg-cnu cg-cnu commented Aug 30, 2017

Description

Url returning 503, fixed it by changing it to rawgit.

Motivation and Context

Fixes #125

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Copy link
Contributor

@kuldeepkeshwar kuldeepkeshwar left a comment

Choose a reason for hiding this comment

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

![build status](https://raw.githubusercontent.com/siddharthkp/bundlesize/master/art/status.png)

<img class="logo" src="https://raw.githubusercontent.com/siddharthkp/bundlesize/master/art/logo.png"/>

I guess we should change these also

@cg-cnu
Copy link
Contributor Author

cg-cnu commented Aug 30, 2017

@kuldeepkeshwar Thanks for the review! I already changed those files as well. Total of 3 files changed.
Can you check and let me know! 🙂
https://github.com/siddharthkp/bundlesize/pull/126/files/9c88c603c53a01bdd0cd785279e2989df9457243

@kuldeepkeshwar
Copy link
Contributor

This is still missing

![build status](https://raw.githubusercontent.com/siddharthkp/bundlesize/master/art/status.png)

Copy link
Contributor

@ForsakenHarmony ForsakenHarmony left a comment

Choose a reason for hiding this comment

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

why is package-lock.json included?

@ForsakenHarmony
Copy link
Contributor

also not sure if we need it in the readme, but ig it won't do any harm

@palashmon
Copy link
Collaborator

palashmon commented Aug 30, 2017

Sidenote: Please don't add Sid as reviewer until further notice, as he is on break for sometime and had unwatched this repo. So, most probably he will not be able to review anything, as mentioned in #121

Regards!

@palashmon palashmon removed the request for review from siddharthkp August 30, 2017 05:39
@cg-cnu
Copy link
Contributor Author

cg-cnu commented Aug 30, 2017

@kuldeepkeshwar Sorry for tagging sid! I completely forgot. I modified the build status in readme.md to rawgit.
@ForsakenHarmony Removed the package-lock from the commit.
Thanks! 😃

@kuldeepkeshwar
Copy link
Contributor

@cg-cnu can you check it again as I can still see package-lock.json in diff

@cg-cnu
Copy link
Contributor Author

cg-cnu commented Aug 30, 2017

@kuldeepkeshwar I thought deleting the file and commiting again will help. I guess I created some conflicts. Can you guide me on this one. Sorry for the trouble. 😞

@SaraVieira
Copy link
Collaborator

@cg-cnu @kuldeepkeshwar I can still see the lock on the diffs

@cg-cnu
Copy link
Contributor Author

cg-cnu commented Aug 30, 2017

@SaraVieira Can you point out how I can remove it ? Or you think I should scrap this and create a fresh pr ?

@SaraVieira
Copy link
Collaborator

@cg-cnu Are you using npm 5?
Get the lock from master, delete all the node modules and then do npm. Install and you should be fine ☺️

@cg-cnu
Copy link
Contributor Author

cg-cnu commented Aug 30, 2017

@SaraVieira Yes am on npm5. Sure will do that. Thanks 😄

@SaraVieira
Copy link
Collaborator

@cg-cnu Awesome! I will update danger to also check for changes in the lock and give you these instructions :)

@SaraVieira
Copy link
Collaborator

@cg-cnu Awesome! I'm going to approve this and as soon as it's merged I'll do a patch release 🎉

@SaraVieira
Copy link
Collaborator

@ForsakenHarmony @kuldeepkeshwar I think it looks great now

@cg-cnu
Copy link
Contributor Author

cg-cnu commented Aug 30, 2017

@SaraVieira Thanks so much for helping me out 😄 Just trying to understand, I shouldn't push the package-lock.json (even if it has changes) unless I made any changes in the package.json and npm installs.

@SaraVieira
Copy link
Collaborator

@cg-cnu I believe so, I will enable greenkeeper in this repo so that we can get warnings when there new releases of stuff so that we are always up to date and that does not happen again :)

@kakadiadarpan
Copy link
Contributor

@SaraVieira Shouldn't we add package-lock.json to .gitignore?

And when we update any package or add a new package, we can explicitly add package-lock.json in the commit.

@SaraVieira SaraVieira mentioned this pull request Aug 30, 2017
@SaraVieira
Copy link
Collaborator

@kakadiadarpan we could but that might bring issues in the future in Travis :/

@reznord
Copy link
Collaborator

reznord commented Aug 30, 2017

we don't even need package-lock.json file. And installing deps on travis hardly takes a minute. So, there is no point in adding either package-lock.json or yarn.lock files to the repo.

I will be opening an issue for the same to discuss on it.

@@ -1,5 +1,5 @@
<p align="center">
<img src="https://raw.githubusercontent.com/siddharthkp/bundlesize/master/art/logo.png" height="200px"/>
<img src="https://cdn.rawgit.com/siddharthkp/bundlesize/master/art/logo.png" height="200px"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

cdn.rawgit redirects to raw.githubusercontent itself. Please revert this change. it is not necessary.

@@ -82,7 +82,7 @@ This makes it great for using with applications that are bundled with another to

#### 2) build status

![build status](https://raw.githubusercontent.com/siddharthkp/bundlesize/master/art/status.png)
![build status](https://cdn.rawgit.com/siddharthkp/bundlesize/master/art/status.png)
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here.

cdn.rawgit redirects to raw.githubusercontent itself. Please revert this change. it is not necessary.

@@ -5,7 +5,7 @@
<link href="https://fonts.googleapis.com/css?family=Nunito" rel="stylesheet">
</head>
<body>
<img class="logo" src="https://raw.githubusercontent.com/siddharthkp/bundlesize/master/art/logo.png"/>
<img class="logo" src="https://cdn.rawgit.com/siddharthkp/bundlesize/master/art/logo.png"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here!

@@ -5,7 +5,7 @@
<link href="https://fonts.googleapis.com/css?family=Nunito" rel="stylesheet">
</head>
<body>
<img class="logo" src="https://raw.githubusercontent.com/siddharthkp/bundlesize/master/art/logo.png"/>
<img class="logo" src="https://cdn.rawgit.com/siddharthkp/bundlesize/master/art/logo.png"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here!

@cg-cnu
Copy link
Contributor Author

cg-cnu commented Aug 30, 2017

@reznord Wait! I am totally lost. I guess the request was to move the urls from raw.githubusercontent to cdn.rawgit. Am I missing something here 😕

@reznord
Copy link
Collaborator

reznord commented Aug 30, 2017

I guess the request was to move the urls from raw.githubusercontent to cdn.rawgit

right. but if you see, cdn.rawgit redirects to raw.githubusercontent itself.

@cg-cnu
Copy link
Contributor Author

cg-cnu commented Aug 30, 2017

The purpose of using rawgit seems to be to get correct content-type

@reznord reznord merged commit f75ab8b into siddharthkp:master Aug 30, 2017
@SaraVieira
Copy link
Collaborator

@reznord @cg-cnu @kuldeepkeshwar Patch release, everyone agree ?

@kuldeepkeshwar
Copy link
Contributor

@SaraVieira 👍

@cg-cnu
Copy link
Contributor Author

cg-cnu commented Aug 30, 2017

@SaraVieira Sounds good 👍

@SaraVieira
Copy link
Collaborator

Done guys:
https://github.com/siddharthkp/bundlesize/releases/tag/v0.14.3

Thanks you so much 🎉

@paulirish
Copy link

@reznord BTW I'm asking for details about this redirect as I didn't expect this behavior. :) rgrove/rawgit#2

@reznord
Copy link
Collaborator

reznord commented Aug 30, 2017

Cool! Will keep a track of it :)

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.

Logo on build info page failing to load
8 participants