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
PWA manifest and new @ini_theme_color replacement #2241
Conversation
This should have been part of the previous commit 5e0255e Some simple static defaults for the PWA manifest
The goal is to make the css replacement accessible in other contexts, for example for the manifest.
This also adds the default theme color for the DokuWiki template as the green from the existing links.
I have extracted some functions from Further, I have added the green from the existing links ( |
define('DOKU_INC', __DIR__ . '/../../'); | ||
} | ||
require_once(DOKU_INC . 'inc/init.php'); | ||
|
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.
Shouldn't this check if the action is enabled before sending the manifest?
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.
Good catch! I'll look into it. If it isn't enabled we send a 400?
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 be a 404 I'd say.
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.
Fixed in ac6ceee
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 like this a lot already. I think the way images are added is still a bit weird. I don't know what formats are required but maybe we could start to use SVG only here? Eg. look for wiki:logo.svg
, :logo.svg
, wiki:dokuwiki.svg
and use whichever we find first only.
inc/load.php
Outdated
@@ -82,6 +82,7 @@ function load_autoload($name){ | |||
'RemoteAPI' => DOKU_INC.'inc/remote.php', | |||
'RemoteAPICore' => DOKU_INC.'inc/RemoteAPICore.php', | |||
'Subscription' => DOKU_INC.'inc/subscription.php', | |||
'StyleUtil' => DOKU_INC.'inc/StyleUtil.php', |
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 shouldn't be needed because the file is namespaced and the autoloader should take care of this.
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.
Also the class seems to be named StyleUtils
?
lib/exe/manifest.php
Outdated
|
||
if (!actionOK('manifest')) { | ||
http_status(404, 'Manifest has been disabled in DokuWiki configuration.'); | ||
return; |
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.
return in global scope feels weird. Should this be exit()
?
inc/confutils.php
Outdated
} | ||
|
||
trigger_error('JSON decoding error "' . $jsonErrorText . '" for file ' . $file, E_USER_WARNING); | ||
return []; |
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 error handling seems excessive. First of all it could probably be better handled with all the error codes in a lookup array (with the JSON* constants as the key).
Then I'm wondering about using trigger_error(). Have you tried what happens when you pass invalid json to json_decode()
? I would assume that it already does trigger an error that would need to be supressed with @
. When we do hande this ourselves I'd think an exception would be the better way?
inc/Manifest.php
Outdated
':wiki:favicon.svg', | ||
':favicon.svg', | ||
'images/favicon.svg', | ||
':wiki:favicon.ico', |
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.
are .ico files supported?
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 haven't read the spec yet, but MDN has them in their example: https://developer.mozilla.org/en-US/docs/Web/Manifest#icons
inc/Manifest.php
Outdated
':favicon.ico', | ||
'images/favicon.ico', | ||
':wiki:logo', | ||
]; |
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.
We're basically adding all of these if we find them, right? It would be nice to fall back to the DokuWiki logo if none could be found.
inc/Manifest.php
Outdated
$look = [ | ||
':wiki:logo.png', | ||
':logo.png', | ||
'images/logo.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.
what does this refer to? There is no images
folder in dokuwiki. There might be one in the template but you wouldn't want that one if you already found one in wiki:logo.png
?
@splitbrain I would like to limit the included icons to a small set of sensible defaults. The user has already many opportunities to overwrite these defaults. The official spec https://www.w3.org/TR/2018/WD-appmanifest-20180222/#icons-member states explicitly that both |
Yes we have a SVG version (see https://www.dokuwiki.org/logo). We could include it next to the png version in the wiki namespace. We could also adjust the template to make use of it? |
Defining SVG both as 17x17 and 512x512 is intended to support custom splash screens, though the documentation is somewhat unclear. This currently satisfies the Google Lighthouse auditing tool.
This color is for example shown in Chrome on Android as menu-bar coloring.
Since StyleUtils has a namespace, it is already handled correctly by the autoloader. This is further underlined by the fact that the removed line isn't actually working because of the missing `s` in the classname.
This isn't really the best place for that error handling.
A recent DokuWiki merge request changed how the style.ini can be accessed: dokuwiki/dokuwiki#2241
Hi,
this is another go at adding an app manifest to DokuWiki in order to provide PWA functionality.
It supports the config cascade and adds a new event
MANIFEST_SEND
.I'm currently not entirely happy with how I select the default images. It is just a list of possible locations and I take the ones I can get.
Further, Google Lighthouse reports that we need an icon with at least 192px to support the automatic installer prompt and an icon of at least 512px for a custom splash screen.
Also, wep apps should have a theme color. I would like to take it from the
style.ini
. Should I use an existing one (which?) or create a new guranteed placeholder in thestyle.ini
for this?The general code structure could still be improved. Maybe move the
Manifest
class toinc/
and thejsonToArray()
method toconutils.php
?replaces #2163
fixes #2156