Fix favicon links to use paths instead of embedded data.#8961
Fix favicon links to use paths instead of embedded data.#8961cjcenizal merged 5 commits intoelastic:masterfrom
Conversation
|
I think you'll need to update the locations in the manifest.json. They need to point to where webpack outputs the images (https://developer.mozilla.org/en-US/docs/Web/Manifest). |
|
Great catch @trevan! Thanks! |
|
We can actually move the assets to |
|
@tylersmalley @trevan Ready for another look, thanks! |
cjcenizal
force-pushed
the
bug/embedded-favicons
branch
from
6292aa3 to
83b2a78
Compare
* Serve favicons directly from ui directory, using exposeStaticDir.
cjcenizal
force-pushed
the
bug/embedded-favicons
branch
from
83b2a78 to
6b1a99b
Compare
|
|
There was a problem hiding this comment.
I'm good with this because it's probably best that the favicons load immediately, rather than waiting for the js assets.
I'd like to point out though; this could be accomplished with webpack and would reduce the spread of favicon-related logic around the source.
If you don't want a file to be inlined in the bundle, or returned as a json object, just prefix the require statement with !file!.
require('!file!./favicon.ico') // loader override with relative pathThe ! syntax is webpack's, and tells it to disable the default loaders and only use the file loader. By default this loader will resolve the require statement with a URL to the required asset that automatically adjusts to basePath and includes the hash of the content for ultimate cache busting.
Here is an example of using this strategy, in case you're interested: https://gist.github.com/spalger/371ebce6782e8335472dc66410ee1307
|
@spalger Thanks man! That's how I originally implemented it in this PR: 5a88701 I decided against this because I think we should solve the problem of "serving static assets" in the same way, wherever possible, to make our code more predictable. I think @tylersmalley has some thoughts about how to improve this in the future. |
|
The main reason for this approach was because we needed to define the path in the manifest.json, which is based on the basePath. I just did some testing in the Android Emulator to verify this PR functionally works. In addition, I tested setting the icon source to be relative, which worked. Considering that is the case, I think it would be best to go back to the static asset and remove the route.
Thoughts? |
|
This LGTM. Another benefit of not having it bundled inline is the reduced bundle size since these assets are primarily browser specific. |
|
LGTM |
* Move favicon files to ui/public/assets/favicons dir. * Serve favicons statically from ui directory, using exposeStaticDir.
* Move favicon files to ui/public/assets/favicons dir. * Serve favicons statically from ui directory, using exposeStaticDir.
* Move favicon files to ui/public/assets/favicons dir. * Serve favicons statically from ui directory, using exposeStaticDir.
… (elastic#9092) * Move favicon files to ui/public/assets/favicons dir. * Serve favicons statically from ui directory, using exposeStaticDir. Former-commit-id: 3396233
6 participants
I originally noticed a problem when I saw that the manifest.json file was being embedded as a JS object. This solution fixes that problem, and additionally sets each href to be a link to the asset instead of the embedded asset.