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

Update README.md #23

Closed
wants to merge 3 commits into from
Closed

Conversation

crispen-smith
Copy link
Contributor

Added additional clarity around module purpose.

Added additional clarity around module purpose.
@dougwilson
Copy link
Contributor

Thank you!

@@ -104,6 +104,8 @@ var server = http.createServer(function onRequest(req, res) {
server.listen(3000);
```


For sites using a more cross-browser solution this module will only provide the default, the serve-static middleware may be a better solution for any additional icons.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what the sentence really means. Can you expand it? This module provides the only cross-browser solution. It woks as far back as IE 5.5 and across all browsers and all operating systems.

@dougwilson
Copy link
Contributor

Perhaps just linking to https://en.wikipedia.org/wiki/Favicon may help people?

@crispen-smith
Copy link
Contributor Author

I think that linking it is a really good idea but I'm sure I can make the phrasing of the highlights more clear let me take another crack at it.

@dougwilson
Copy link
Contributor

I'm in no rush :)

@adius
Copy link

adius commented Feb 3, 2015

This should be merged! Just stumbled upon the same problem as described in #22.
(…but please fix the typos first ;-) )

@dougwilson
Copy link
Contributor

I did not realize the PR had been updated.

@crispen-smith
Copy link
Contributor Author

Reviewed again and made a few minor additional changes.

@dougwilson
Copy link
Contributor

Looks good. Will merge :) Sorry for the delay; I just didn't realize it was updated before :O

@crispen-smith
Copy link
Contributor Author

Just glad I was able to help.

@dougwilson dougwilson closed this in e76bc20 Feb 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants