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

docs: New favicons and add a service worker #23101

Merged
merged 2 commits into from
Aug 15, 2017
Merged

Conversation

XhmikosR
Copy link
Member

Include everything for most browsers to work including Android.

Kept favicon.ico to the root dir.

/CC @mdo @bardiharborow @Johann-S

@Johann-S
Copy link
Member

Overlapping with #22895 but I prefer this way to update our favicon 👍

@XhmikosR
Copy link
Member Author

Doesn't overlap because I kept favicon.ico in root for this reason :)

@XhmikosR XhmikosR force-pushed the v4-dev-xmr-docs-favicons branch 2 times, most recently from c75ff77 to 39f62cc Compare July 16, 2017 13:56
@XhmikosR XhmikosR changed the title New favicons. docs: New favicons and add a service worker Jul 16, 2017
@XhmikosR
Copy link
Member Author

Added a service worker.

@Johann-S
Copy link
Member

What's the goal of this Service Worker ?

@XhmikosR
Copy link
Member Author

https://developers.google.com/web/progressive-web-apps/

First thing I want to make https://developers.google.com/web/updates/2017/02/improved-add-to-home-screen work and then we can look into the offline stuff.

@mdo
Copy link
Member

mdo commented Jul 16, 2017

#22895 was slated for Beta 2, not Beta 1. Similarly, this PR isn't in scope for our first beta release either. Before we attempt to merge, let's better understand what this is and why we need it.

As an aside, 3% of our docs traffic is from Android. I have a hard time justifying new code and optimizations there given the audience size. Not saying that PWA are bad in general, just that we don't really gain much of anything by doing this.

@XhmikosR XhmikosR force-pushed the v4-dev-xmr-docs-favicons branch from 39f62cc to 2c627b7 Compare July 31, 2017 06:27
@XhmikosR
Copy link
Member Author

@mdo: this isn't only for Android, since PWAs are the future. It's just that currently that's where it works.

This is a nice addition and along with offline capabilities which I plan to add, would help reading our docs offline etc.

@XhmikosR XhmikosR force-pushed the v4-dev-xmr-docs-favicons branch 2 times, most recently from 1a07307 to 539e66e Compare August 11, 2017 10:05
@XhmikosR XhmikosR requested review from mdo and Johann-S August 11, 2017 10:17
@@ -0,0 +1 @@
// empty for now
Copy link
Member

Choose a reason for hiding this comment

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

Is it ok to merge an empty file ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, otherwise it's a 404. This is until we actually add any code for service workers.

@XhmikosR XhmikosR force-pushed the v4-dev-xmr-docs-favicons branch from d5fcbc7 to 7b99c46 Compare August 15, 2017 11:28
Include everything for most browsers to work including Android.

Kept favicon.ico to the root dir.

Also added a manifest.json for PWA.
@XhmikosR XhmikosR force-pushed the v4-dev-xmr-docs-favicons branch from 7b99c46 to 20abbc3 Compare August 15, 2017 11:36
@XhmikosR XhmikosR merged commit ac718c9 into v4-dev Aug 15, 2017
@XhmikosR XhmikosR deleted the v4-dev-xmr-docs-favicons branch August 15, 2017 12:00
@mdo mdo mentioned this pull request Aug 18, 2017
@mdo mdo mentioned this pull request Jul 25, 2018
1 task
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.

3 participants