Add support for favicon #32

SimenB · 2015-05-08T17:28:59Z

This is supposed to be a temporary solution until #10 is implemented.

A file named favicon.ico has to be included manually in some require, which I suppose is bad, but until #10 is done, I don't see any way around it

SimenB · 2015-05-08T17:29:36Z

index.js

+      return /^favicon\.ico($|\?)/.test(path.basename(module.name));
+    });
+    if (assets.favicon) {
+      assets.favicon = webpackStatsJson.publicPath + assets.favicon.assets[0];


I don't know if webpack has some nice util that will make this less manual?

jantimon · 2015-05-09T07:45:28Z

Unfortunately this does not work with the new template mode (inject: true).

jantimon · 2015-05-09T07:46:16Z

Why do we have to set favicon to true?
Wouldn't it be enough to look for favicons in the assets object?

SimenB · 2015-05-09T07:50:00Z

Unfortunately this does not work with the new template mode (inject: true).

Ah, didn't think of that. I'll give it a whack 😄

Why do we have to set favicon to true?
Wouldn't it be enough to look for favicons in the assets object?

The thought is that it is a potentially heavy operation (my project at work has over 1000 assets). I suppose it's not too much either way, node is pretty fast... What do you think?

jantimon · 2015-05-09T07:55:24Z

We do sth similar with the manifest.
It would be cool if we could keep the same style for the favicon:
https://github.com/SimenB/html-webpack-plugin/blob/favicon/index.js#L86

SimenB · 2015-05-09T08:01:44Z

@jantimon Added support for inject. I'll try copying the way manifest is retrieved

jantimon · 2015-05-09T08:14:43Z

Would it be possible to specify a path for favicon in the options and load it like
compilation.assets['favicon.ico'] = faviconObject. (similar to https://github.com/lettertwo/appcache-webpack-plugin/blob/master/src/index.coffee)
If I understood this correct this would allow to add a favicon without requiring the file manually.

jantimon · 2015-05-09T08:22:15Z

It would also be nice if this also allowed to specify files for apple-touch-icon

SimenB · 2015-05-09T08:35:02Z

@jantimon Unless I want to include any .ico file as an icon, I can't use the same way the manifest is received, as compilation.assets only has the name Webpack gives the file, not the original.

I'll see if I can get it working without manually requiring. I can then add the same logic for apple-touch-icon. Coffeescript is like reading Chinese to me...

SimenB · 2015-05-09T10:02:14Z

@jantimon I think that we can't use how appcache-webpack-plugin does it, as the favicon is an external file, and not something I build up manually.

SimenB · 2015-05-09T11:00:30Z

@jantimon Added support for apple-touch-icon. It has to be named apple-touch-icon.png. This is the name h5bp has given it.

I also removed the option, it's now automatically picked up

SimenB · 2015-05-09T14:12:25Z

@jantimon I expanded the PR to also cover any extra file you may want. I can separate it into it's own PR if you'd like, but it builds on this one, so I thought I'd keep them together

jantimon · 2015-05-09T14:50:53Z

yeah I guess the extra file should be in an extra pull request for now.

SimenB · 2015-05-09T15:11:08Z

@jantimon Ok, removed those changes

SimenB · 2015-05-09T15:44:02Z

@jantimon Found a bug if an image was base64-encoded. Should be ready for review now 😄

SimenB · 2015-05-09T16:38:23Z

index.js

+        return [path.parse(module.name).name, publicPath + module.assets[0]];
+      }
+
+      return [path.parse(module.name).name, module.source.substring(18, module.source.length - 1)];


module.exports = " as well as the last "

SimenB · 2015-05-10T16:40:37Z

@jantimon Any chance this could get merged (or feedback, if it's missing something)? If you're enjoying the weekend, ignore my nagging 😄

I'd like to see this in before tomorrow, as I'll need it at work, though.

SimenB · 2015-05-10T16:42:46Z

index.js

@@ -70,6 +70,27 @@ HtmlWebpackPlugin.prototype.emitHtml = function(compilation, htmlTemplateContent
  };
 };

+HtmlWebpackPlugin.prototype.getAssetPathFromModuleName = function(publicPath, modules) {


It might be better to extract some if these functions into a lib/ directory or something, as this file is getting a bit bulky, and there's no real need to export anything but the constructor and the apply-function. I can make a separate PR moving stuff around though

Fixes #28

jantimon · 2015-05-11T19:26:26Z

Hi @SimenB thanks for all your work. Unfortunately I am on a conference for the next days but I'll take a look at it.
By now you can simply install it with npm i --save-dev SimenB/html-webpack-plugin

SimenB · 2015-05-11T19:44:26Z

@jantimon Ah, OK. The proxy at work kills GitHub dependencies. I'll just publish a fork in the meantime, then deprecate when (if) you merge it 😄

jantimon · 2015-05-11T21:35:11Z

Here is a first try to solve the favicon (based on your changes)
https://github.com/ampedandwired/html-webpack-plugin/tree/favicon

I removed the apple icon for now as it didn't allow to specify multiple dimensions e.g.

<link rel="apple-touch-icon" sizes="76x76" href="/apple-touch-icon-76x76.png" />
<link rel="apple-touch-icon" sizes="120x120" href="/apple-touch-icon-120x120.png" />
<link rel="apple-touch-icon" sizes="152x152" href="/apple-touch-icon-152x152.png" />

Another big change was that it uses a similar way to add the favicon.ico like the app manifest plugin.
That means that you don't have to require the favicon.ico in your js code and it won't be part of the resulting output javascript file.

What do you think?

SimenB · 2015-05-11T21:53:59Z

@jantimon Oooh, those changes look much cleaner! Great job!
Could you also expand on the idea I have going in #36, and make it easy to include any arbitrary file, and not just favicon? Your addFileToAssets should make that pretty easy.
Will files added to the compilation this way be passed through loaders? As in getting base64-encoded assets if through url-loader etc.

Regarding apple icons, I thought about the different sizes, but as h5bp removed them (h5bp/html5-boilerplate#1367), and now recommend using just one (h5bp/html5-boilerplate#1622), I thought it was fine to just include the one size.

jantimon · 2015-05-12T21:29:18Z

Although I really like the idea with the extra files I am not sure wether this is just an edge case or a common use case. It is already possible to pass a custom templateContent function which has access to everything needed to use these files.

You are correct the base64 encoding is missing as it doesn't use the loaders.
However I would really prefer not to load the favicon into the javascript.
Maybe you could come up with a good solution for this issue?

The h5bp is referring only to the file name and is a solid starting point for creating web sites however imho there are still use cases where you would like to provide different icon versions depending on the size.

I rebase this pr into #37 for the new 1.4.0 release but I would like to get the feedback of @ampedandwired before merging.

SimenB · 2015-05-12T21:35:19Z

The us case behind extra files is that I don't want to know how to iterate through the files in my template, or how Webpack inner structure works. I just want this and that file in my template.

I have no idea how to send it through loaders, ref #10. Would be awesome though!

As my use case for this was just favicon, I think it is covered by #37. I'd like to see if we can reach a consensus on #36 though. I'll rebase after 1.4.0 is released 😄

lock · 2018-05-31T16:26:59Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

SimenB reviewed May 8, 2015
View reviewed changes

SimenB mentioned this pull request May 9, 2015

Added support for minification using html-minifier #20

Closed

SimenB mentioned this pull request May 9, 2015

Add support for specifying any files for easy access in templates. #36

Closed

SimenB reviewed May 9, 2015
View reviewed changes

SimenB reviewed May 10, 2015
View reviewed changes

Add support for favicon and apple-touch-icon

5771dac

Fixes #28

jantimon mentioned this pull request May 12, 2015

1.4.0 #37

Merged

SimenB closed this May 12, 2015

SimenB deleted the favicon branch May 12, 2015 21:35

lock bot locked as resolved and limited conversation to collaborators May 31, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for favicon #32

Add support for favicon #32

SimenB commented May 8, 2015

SimenB May 8, 2015

jantimon commented May 9, 2015

jantimon commented May 9, 2015

SimenB commented May 9, 2015

jantimon commented May 9, 2015

SimenB commented May 9, 2015

jantimon commented May 9, 2015

jantimon commented May 9, 2015

SimenB commented May 9, 2015

SimenB commented May 9, 2015

SimenB commented May 9, 2015

SimenB commented May 9, 2015

jantimon commented May 9, 2015

SimenB commented May 9, 2015

SimenB commented May 9, 2015

SimenB May 9, 2015

SimenB commented May 10, 2015

SimenB May 10, 2015

jantimon commented May 11, 2015

SimenB commented May 11, 2015

jantimon commented May 11, 2015

SimenB commented May 11, 2015

jantimon commented May 12, 2015

SimenB commented May 12, 2015

lock bot commented May 31, 2018

Add support for favicon #32

Add support for favicon #32

Conversation

SimenB commented May 8, 2015

SimenB May 8, 2015

Choose a reason for hiding this comment

jantimon commented May 9, 2015

jantimon commented May 9, 2015

SimenB commented May 9, 2015

jantimon commented May 9, 2015

SimenB commented May 9, 2015

jantimon commented May 9, 2015

jantimon commented May 9, 2015

SimenB commented May 9, 2015

SimenB commented May 9, 2015

SimenB commented May 9, 2015

SimenB commented May 9, 2015

jantimon commented May 9, 2015

SimenB commented May 9, 2015

SimenB commented May 9, 2015

SimenB May 9, 2015

Choose a reason for hiding this comment

SimenB commented May 10, 2015

SimenB May 10, 2015

Choose a reason for hiding this comment

jantimon commented May 11, 2015

SimenB commented May 11, 2015

jantimon commented May 11, 2015

SimenB commented May 11, 2015

jantimon commented May 12, 2015

SimenB commented May 12, 2015

lock bot commented May 31, 2018