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
CB-12804 : support manifest.json #32
Conversation
…ad of webkitpagevisibility
…writing to manifest and uses platform_www/
…it if nothing else registers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Jesse, tried to go over this as closely as possible. I have some comments, mostly for discussion, nothing that should hold up this PR.
} | ||
else { | ||
var manifestJson = { | ||
"background_color": "#000", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering if background_color
default should be white instead. Not opposed to black as it is probably close to 50/50.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to white in #36
"type": "image/png", | ||
"sizes": "128x128" | ||
} ******/ | ||
// ?Is it worth looking at file extentions? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think it makes sense to look at the file extension to figure out the mimeType. Some folks might use jpg for their icons. In that case we should warn folks that png is the preferred format (that is based on something I remember reading from Google).
var oriPref = this.config.getGlobalPreference('Orientation'); | ||
if(oriPref && ["landscape","portrait"].indexOf(oriPref) > -1) { | ||
manifestJson.orientation = oriPref; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if Orientation
is set to default
we should probably set the manifest value to any
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all other values will be mapped to 'any' in #36
} | ||
else { // see if there is a preference in config.xml | ||
// <preference name="StatusBarBackgroundColor" value="#000000" /> | ||
themeColor = this.config.getGlobalPreference('StatusBarBackgroundColor'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we ignore StatusBarBackgroundColor
if it is set to ['black', '#000', '#000000']?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would we ignore some values? What is wrong with a black statusbar?
|
||
this.addEventListener('fetch', function(event) { | ||
console.log("cordova service worker : fetch : " + event.request.url); | ||
event.respondWith(caches.match(event.request)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the examples I've seen from Google program this a bit more defensively:
event.respondWith(
caches.match(event.request)
.then(function(response) {
// Cache hit - return response
if (response) {
return response;
}
return fetch(event.request);
}
)
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defensively added to #36
"sizes": "192x192", | ||
"type": "image/png" | ||
}, { | ||
"src": "img/splash.png", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need the splash in the list of icons. Chrome will use the background_color, short_name and icon closest to 128dp to create a splash screen.
@@ -61,14 +61,9 @@ function readConfig(success, error) { | |||
} | |||
}; | |||
|
|||
if ("ActiveXObject" in window) { | |||
// Needed for XHR-ing via file:// protocol in IE | |||
xhr = new window.ActiveXObject("MSXML2.XMLHTTP"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hahahahahaha, I'm so old.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm older ( not a contest )
@@ -1486,6 +1486,26 @@ module.exports = { | |||
|
|||
bootstrap: function() { | |||
|
|||
var cache = navigator.serviceWorker.register; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a great solution for detecting if a service worker has been registered.
…ues to 'any', address some issues raised in apache#32
Platforms affected
cordova-browser
What does this PR do?
adds manifest.json support
What testing has been done on this change?
added tests
This pr includes all commits from #30 which can be closed.
Checklist