docs(readme): fixed readme images to link to local assets #6116
Conversation
|
hmm does npm fix the image path when it displays the readme? |
Good point, yeah that can be an issue. Switched up to absolute paths so that they are universal. |
readme.md
Outdated
| @@ -10,7 +10,7 @@ Lighthouse is integrated directly into the Chrome Developer Tools, under the "Au | |||
|
|
|||
| **Run it**: open Chrome DevTools, select the Audits panel, and hit "Run audits". | |||
|
|
|||
| <img width="500px" alt="Lighthouse integration in Chrome DevTools" src="https://user-images.githubusercontent.com/2301202/40556658-4ef7d128-6002-11e8-903e-5224894a7cca.png"> | |||
| <img src="https://github.com/GoogleChrome/lighthouse/raw/master/assets/example_dev_tools.png" alt="Lighthouse integration in Chrome DevTools" width="500px"> | |||
There was a problem hiding this comment.
Is there a reason we're moving to raw/master instead of hash-based?
raw/master can just be super confusing once master gets ahead of older versions. We don't update the images that often, think we could stick with hashes?
There was a problem hiding this comment.
hm true
https://www.npmjs.com/package/@code-therapy/packages-with-readme-relative-images#conclusion lays out how annoying this is right now
There was a problem hiding this comment.
Hmm I definitely find the hashed links confusing because I never know where they are coming from. And I think it would be much easier to keep them updated if they were in the repo/easy to find and update.
There is the raw.github link for these images e.g. https://raw.githubusercontent.com/GoogleChrome/lighthouse/master/assets/lighthouse-logo.png but I guess that comes right back to the raw/master issue, this is just the resolution after replacing blob with raw.
Not sure exactly how to handle this then.
There was a problem hiding this comment.
Seems like in the example @paulirish sent over he uses the form raw.githubusercontent.com/.../HEAD/.../...png so it removes the master hardcoding. Seems like what he was doing might be good for us to maintain absolute paths, but still have some flexibility.
… from raw/github.
readme.md
Outdated
| @@ -10,7 +10,7 @@ Lighthouse is integrated directly into the Chrome Developer Tools, under the "Au | |||
|
|
|||
| **Run it**: open Chrome DevTools, select the Audits panel, and hit "Run audits". | |||
|
|
|||
| <img width="500px" alt="Lighthouse integration in Chrome DevTools" src="https://user-images.githubusercontent.com/2301202/40556658-4ef7d128-6002-11e8-903e-5224894a7cca.png"> | |||
| <img src="https://raw.githubusercontent.com/GoogleChrome/lighthouse/HEAD/assets/example_dev_tools.png" alt="Lighthouse integration in Chrome DevTools" width="500px"> | |||
There was a problem hiding this comment.
I'm not 100% sure what ../HEAD/ will do some maybe you can help me understand :)
I'll restate what I'm worried about too. Scenario:
- We publish v3.x with
../HEAD/..(or master) links. - We continue working and update the images or remove them entirely.
- HEAD and master now point to a different image or is broken link.
I probably missed some magic described in that doc but seems like we still need a specific tag or hash in there for them to continue to work. Not as big a deal as doc links IMO but should probably iron out if we're switching them all anyhoo :)
There was a problem hiding this comment.
HEAD is basically an alias to the default branch, which is master for us. It's just a way of taking out master from the url IIRC, but I can't find any supporting docs 😦
That is a valid concern. If you look at the readme on this branch you can tell it is broken b/c example_dev_tools.png doesn't exist on master yet.
Looking into what github has for permanent links we could do that e.g. https://raw.githubusercontent.com/GoogleChrome/lighthouse/e7997b3db01de3553d8cb208a40f3d4fd350195c/assets/example_dev_tools.png link to the file as seen on a specific commit, thus guaranteeing it never breaks going forward. But it opens it up to getting stale and the update procedure is more complicated than just sticking in a new image to the assets folder.
I think that if we do change it to this, or even keep it the same, we need to document this process b/c for someone like me with the user-content hash'd URLs I had no idea how to update them. So I think moving to something like this, where the images exist and have discernible URLs would be helpful IMO.
|
let's just go with something since this def isn't worth the time we're sinking on it. :) I'd be fine with "something" is closing the PR. After all, NPM does currently render the readme just fine. Second preference is to use gitubusercontent absolute URLs that we know will render reliably always. |
|
Either of those SGTM, didn't mean to cause a thing! :) |
|
Okay, I think I got it to a point where we can all be happy. Images point to files using an absolute path that is tied to the commit's hash. So they will never break. But also are tied to the image b/c the url has the image name and path in it. I also documented this in CONTRIBUTING so that the process can be repeated! |
Summary
Fixed the main readme linking to images outside the repo. Added local copy of dev tools image. Linked footer icon to local png icon. Made example report 500px max.