Favicons 4 #51
Comments
|
Let's assume ImageMagick and GM are out of the question due to their age and dependencies. Remove Pica as (from the perspective of a JS-only library) JIMP appears more popular. So either Sharp or JIMP I believe. As per Sindre's comment:
I think JIMP is the way to go. |
|
I never used JIMP, but it make sense to use pure JS. Note that, if you want to support the new Safari pinned tabs, you'll need to generate SVG, which JIMP apparently doesn't support. |
|
From my perspective as a library author consuming this library as a dependency, I'd love to see a couple things in particular:
|
|
I would love to see a more sane config format with general configuration options and vendor specific sections. I also agree with @davewasmer about the file-writing - that was my major reason for requesting (and implementing) extra options - I needed them solely to configure how the |
|
@davewasmer Good idea. I think the Favicons callback will look like this: var cheerio = require('cheerio');
function (error, images, html) {
// error: any error that occurred in the process (string)
if (error) throw error;
// images: an object of buffers e.g. { apple-touch-icon.png: <Buffer>, favicon.ico: <buffer> }
for (var favicon in images) {
if (images.hasOwnProperty(favicon)) {
fs.writeFileSync('dist/' + favicon, images[favicon]);
}
}
// html: a snippet of HTML (string) e.g.
$ = cheerio.load('index.html', { decodeEntities: false });
$('head').append(html);
}I definitely agree on adding this to the build tools but it seems like quite a bit of work for the Node module. Maybe we can have an optional write? Gulp will be something like: gulp.task('default', function () {
gulp.src('logo.jpg')
.pipe(favicons({
background: '#1d1d1d' // Normal variables
html: 'index.html' // Gulp-specific variables
}))
.pipe(gulp.dest('favicons/'));
}); |
|
@euangoddard My current concept for the configuration (with defaults) is this: {
"appName": null,
"appDescription": null,
"developer": null,
"developerURL": null,
"background": "#fff",
"path": "./",
"silhouette": false,
"version": 1.0,
"logging": false,
"online": false,
"icons": {
"android": true,
"appleIcon": true,
"appleStartup": true,
"coast": true,
"favicons": true,
"firefox": true,
"opengraph": true,
"windows": true,
"yandex": true
}
}Not entirely sure on the specifics like "silhouette" as it depends on the final output, but what are your thoughts on this? |
|
@haydenbleasel Looks great. A couple suggestions, off the top of my head: // error: any error that occurred in the process (string)
if (error) throw error;I'd favor an For example, if I elect to use the API, and the API returns an error like // images: an object of buffers e.g. { apple-touch-icon.png: <Buffer>, favicon.ico: <buffer> }
for (var favicon in images) {
if (images.hasOwnProperty(favicon)) {
fs.writeFileSync('dist/' + favicon, images[favicon]);
}
}Personally, I think an array of
Not sure what you mean here - that your callback snippet is too much work? I think that is fine - I'm guessing most people wouldn't consume this library directly (but I'm admitted biased here, as a consuming lib author 😉) |
|
@davewasmer Good idea on the errors, is there a specific error object style I should use? Otherwise I'm thinking As for the images callback, I suppose there's not much difference with the following except for simpler syntax: images.forEach(function (favicon) {
fs.writeFileSync('dist/' + favicon.name, favicon.contents);
}); |
|
@haydenbleasel for the structure, I don't think it matters a ton, as long as (a) it's consistent to whatever extent possible (i.e. if it's a filesystem error, I wouldn't expect an HTTP |
|
@davewasmer Okay sounds good, here's the error template we'll be using: {
status, // HTTP Status Code or null if filesystem error
error, // Enum or title, to be discussed in detail
message // Description of the error, fallback to "An unknown error has occurred"
} |
|
@haydenbleasel that config is so much clearer to me - a big win. My only comment would be that runtime configuration is mixed with outcome-based configuration (i.e. |
|
@euangoddard Yeah the current favicons config is categorised but that causes too much work for simple parameters e.g. Basic configuration and disabling 1 icon type: {
files: {
src: a,
dest: b,
html: c
},
icons: {
appleIcon: false
},
settings: {
appName: d,
appDescription: e,
developer: f,
}
}New one: {
appName: d,
appDescription: e,
developer: f,
html: c,
icons: {
appleIcon: false
}
} |
|
Does anyone know if there's a pure Javascript npm package for convering SVG to a rasterized image type like JPG / PNG / BMP? |
|
Although it's a bit strange to me, the new concept for files is to return something like: {
name: 'browserconfig.xml',
contents: [
'<square70x70logo src="small.png"/>',
'<square150x150logo src="medium.png"/>',
'<wide310x150logo src="wide.png"/>',
'<square310x310logo src="large.png"/>',
'<TileColor>#009900</TileColor>'
]
}That way it's consistent with the way I return HTML (just the contents, no wrapper). In the Node module, the user is left to handle that output and append it to an existing |
|
@phbernard I think we need proper unit tests for this new version, you any good with that? |
|
Yup! Don't count on me to write hundreds of tests for every aspects of the module, but I can surely write some to get started. Which parts would you like to focus on? |
|
Haha yeah of course mate :) I guess we're generating 3 seperate components at the moment (images, files, html) so maybe just checking that the correct files have been created / the correct HTML has been produced? What do you reckon? |
|
Sounds good. I'll code some mocha tests, I think tomorrow. |
|
Sounds good! Thanks mate :) |
|
@haydenbleasel looking at that output - are you saying that the file contents will be an array of strings? |
|
Hey @davewasmer, updated callback is as follows: function (error, response) {
console.log(error.status); // HTTP error code or null
console.log(error.name); // e.g. "API Error"
console.log(error.message); // e.g. An unknown error has occurred
console.log(response.images); // Array of { name: string, contents: <buffer> }
console.log(response.files); // Array of { name: string, contents: <buffer> }
console.log(response.html); // Array of strings (html elements)
}); |
|
ah, makes sense 👍 |
|
Okay so I'm working on the new RFG integration now, there's two ways of handling the image/file response cc: @davewasmer @phbernard. The first is the traditional way, we'd have to request the zip file, unzip it and pass the files into an array or something: The second is downloading each image individually, decreasing request size and avoiding the whole unzipping process but increases the amount of HTTP requests: Thoughts? |
|
I'd vote for the latter (separate requests). Since this is a Node lib, it won't be subject to the browser's concurrency restrictions, so firing off a many simultaneous requests shouldn't be a problem. On the other hand saving the bandwith and avoiding the whole unzipping problem seems very attractive. Sent from my iPhone
|
|
@davewasmer Agreed. We can asynchronously grab each one of the files, should also avoid timeout issues and finish quicker. |
|
Also @phbernard I totally forgot to respond to that last message re: SVG support for El Capitan, see #53 as I'm not sure we can get it into |
This is a complete rewrite of Favicons to suit the pressing needs of users and fix a lot of bugs. I think I've lost track of the original use case for Favicons over time: "I want to generate a complete suite of favicons and valid HTML with the smallest amount of work possible". Let's create a discussion of what should be in the new version:
We can only use it with one html file at a time(gulp-favicons)Add SVG support i.e. Favicons integration adaptdk/svg2favicons#1(no pure-js svg-to-png Node.js module available, also applies to that new El Capitan SVG thing...)Create a Grunt version (naming conflict: https://github.com/gleero/grunt-favicons)(Gleero won't respond)If you'd like to collaborate physically, check out the
favicons-4branch.The text was updated successfully, but these errors were encountered: