-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
A minimal App Install Banner demo #83
Conversation
Merge in all the latest latest jazz.
Nice work, Paul. Happy to review further. Could you squash your commits please? :) I have a few questions (e.g why we're using |
How does one squas commits in the Chrome Dev Tools editor and in the github interface? |
Does CDT support git commands? If so, http://blog.steveklabnik.com/posts/2012-11-08-how-to-squash-commits-in-a-github-pull-request |
Updating some bits Simple fix Some minor changes. test The bug in Chrome has been fixed, returning to 'type'
It does not, I pulled it all in locally on another machine and squashed there instead. I can't get rid of the merge commit. |
Nope. however it was copied from one of the SW examples. Will sort. |
Reviewed. One other minor comment: we usually include a brief description of the feature https://github.com/GoogleChrome/samples/blob/gh-pages/lexical-declarations-es6/index.html#L49 along with its name (heading) and any other posts or ChromeStatus links (https://www.chromestatus.com/features/4540065577435136) we want to reference at the start of the sample. Also mention which version of Chrome and above one can expect this to work in. Could we do this? |
Sure I can do this, I mostly copied the formatting and style from another example. Will add them. |
Thank you 🍰 |
One last comment. |
Sorted typo |
A minimal App Install Banner demo
👍 |
This went through a couple of changes as I worked with the eng team to diagnose a critical bug.
This now works. @addyosmani @jeffposnick would love a review so I can get it in to the post about this feature.