Admin Page refactoring #1809

splitbrain · 2017-01-21T13:35:05Z

This takes #1767 as a starting point to refactor the whole Admin screen:

new Ui/Admin class instead of a monolithic function
SVG icons styled through CSS
support for plugin icons
new embedSVG() method to be reused in additional refactorings
use of less instead of plain css

this is part of an alternative implementation to #874

Squashed commit of the following: commit 86183b66c5b53b47e5ddb1e0d1c155c06c331d35 Merge: ebfb1ab 4a8f428 Author: Andreas Gohr <andi@splitbrain.org> Date: Sat Jan 21 11:20:32 2017 +0100 Merge branch 'master' of git://github.com/ThisNameIsNotAllowed/dokuwiki into pull-request-1767 * 'master' of git://github.com/ThisNameIsNotAllowed/dokuwiki: Update admin.php Update admin.php Update _admin.css Update admin.php Update html.php Update _admin.css Update html.php Update html.php Update html.php Update.html Update html.php Update admin.php commit 4a8f428 Author: ThisNameIsNotAllowed <ThisNameIsNotAllowed@users.noreply.github.com> Date: Fri Jan 20 09:25:52 2017 +0100 Update admin.php Changed the code to check for icons being SVG files. This should fix php5.3 errors for using method returns straight on. commit e43b6ca Author: ThisNameIsNotAllowed <ThisNameIsNotAllowed@users.noreply.github.com> Date: Fri Jan 20 08:22:41 2017 +0100 Update admin.php Removed elements that could lead to behaviors that shouldn't occure. commit 9af82fb Merge: b99c677 0b8d187 Author: ThisNameIsNotAllowed <ThisNameIsNotAllowed@users.noreply.github.com> Date: Thu Jan 19 16:22:02 2017 +0100 Merge pull request #1 from ThisNameIsNotAllowed/add-c Added css, forced the author to use ".svg"-files for plugin icons. commit 0b8d187 Author: ThisNameIsNotAllowed <ThisNameIsNotAllowed@users.noreply.github.com> Date: Thu Jan 19 16:17:34 2017 +0100 Update _admin.css fixed some aligning for plugin names. commit 9158649 Author: ThisNameIsNotAllowed <ThisNameIsNotAllowed@users.noreply.github.com> Date: Thu Jan 19 15:49:20 2017 +0100 Update admin.php Added method to return menu icons only in case their mime type matches svg files. Added warning to getMenuIcon comments telling the user to provide svg only. commit 6df4e0f Author: ThisNameIsNotAllowed <ThisNameIsNotAllowed@users.noreply.github.com> Date: Thu Jan 19 15:46:17 2017 +0100 Update html.php Added code to accept svg files only. Also changed names of css classes to suit dokuwiki style. commit 3435abc Author: ThisNameIsNotAllowed <ThisNameIsNotAllowed@users.noreply.github.com> Date: Thu Jan 19 15:41:06 2017 +0100 Update _admin.css Added css for displaying plugin icons. commit b99c677 Author: ThisNameIsNotAllowed <ThisNameIsNotAllowed@users.noreply.github.com> Date: Thu Nov 24 14:16:23 2016 +0100 Update html.php Changed the list of admin plugins. The icons and plugin names appearance can now be influenced by css. commit 479c651 Author: ThisNameIsNotAllowed <ThisNameIsNotAllowed@users.noreply.github.com> Date: Thu Nov 24 13:45:08 2016 +0100 Update html.php Cleaned, due to too much duplicated code. commit e621fd9 Author: ThisNameIsNotAllowed <ThisNameIsNotAllowed@users.noreply.github.com> Date: Wed Nov 23 16:26:14 2016 +0100 Update html.php Changed class name for images shown before plugin names. commit 9099dac Author: ThisNameIsNotAllowed <ThisNameIsNotAllowed@users.noreply.github.com> Date: Wed Nov 23 16:24:01 2016 +0100 Update.html Added icon to plugin array commit faea3ce Author: ThisNameIsNotAllowed <ThisNameIsNotAllowed@users.noreply.github.com> Date: Wed Nov 23 15:56:40 2016 +0100 Update html.php Added support to display plugin icons in admin menu. commit 539e60b Author: ThisNameIsNotAllowed <ThisNameIsNotAllowed@users.noreply.github.com> Date: Wed Nov 23 15:51:39 2016 +0100 Update admin.php Add support for icons in admin menu.

This introduces an embedSVG() function that can be used at other places.

This introduces a new dokuwiki\Ui namespace and refactors the Admin screen into a Ui class. The ultimate goal is to split up the big, complex functions in inc\html.php in better maintainable classes in the Ui namespace. This is the first go at it. Others function->class conversions should follow. This also switches the icons for our base admin plugins to inline SVG. (files and styling not included, yet).

more complex material design icons can be somewhat larger than 1kb

using icons from https://materialdesignicons.com/

mention-bot · 2017-01-21T13:35:08Z

@splitbrain, thanks for your PR! By analyzing the history of the files in this pull request, we identified @selfthinker, @adrianheine and @Klap-in to be potential reviewers.

selfthinker · 2017-01-29T19:16:52Z

inc/Ui/Admin.php

+     */
+    protected function showPluginMenu() {
+        if(!count($this->menu)) return;
+        echo '<div class="clearer"></div>';


We should try to not use the clearer div anymore. Use the group class in the surrounding container instead.

actually the clearer isn't even needed here

selfthinker · 2017-01-29T19:18:25Z

inc/Ui/Admin.php

+     */
+    public function show() {
+        $this->menu = $this->getPluginList();
+        echo '<div class="UI-admin">';


We don't use CSS classes with uppercase letters anywhere else. I find it can create confusion and be more error-prone and there might be problems with some older browsers.

selfthinker · 2017-01-29T19:21:50Z

inc/Ui/Admin.php

+        global $ID;
+        if(blank($item['prompt'])) return;
+        echo '<li><div class="li">';
+        echo '<span>';


Why is this in a span and not within the link? The icon is blue which indicates it's part of the link but it isn't. That is confusing. I would either make the colour not blue or put the icon inside the link to be clickable as well.

That span is empty if there is no icon. Can we avoid that?

I don't know how. We want the spacing when there's no icon.

selfthinker · 2017-01-29T19:22:51Z

inc/Ui/Ui.php

+/**
+ * Class Ui
+ *
+ * Abstrract base class for all DokuWiki screens


Can you explain what your thinking is behind the UI class?
(Minor typo: "Abstrract")

I just wanted a base for all the UI screens to be able to add base functionality later on (not sure what that functionality would be).

And what do you mean by "UI screens"?

Well currently there's only one. But I want to replace much of inc/html.php. so ultimately we'd have things like Ui\Diff, Ui\MediaManager, Ui\Recents etc.

Okay, thanks, that makes sense.

selfthinker · 2017-01-29T19:38:31Z

lib/tpl/dokuwiki/css/_admin.less

+        list-style-type: none;
+        font-size: 1.125em;
+    }
+    [dir=rtl] ul.admin_tasks {


That won't work. You would need to repeat the parent class with &, like [dir=rtl] & ul.admin_tasks. (Not tested. That would be true for SASS, not 100% sure if the syntax is correct for LESS.)
That would obviously be the same for all further occurrences below.

Hmm I thought I tried it. Gotta check again.

Yeah you're right. The & trick works though.

selfthinker · 2017-01-29T20:10:18Z

lib/tpl/dokuwiki/css/_admin.less

+        list-style-type: none;
+
+        div {
+            // flexbox takes care of RTL alignment


What is the reason for using flexbox? Because the SVG is inline, it works fine in older browsers, so that's not an issue. I assume you would use it to make the spacing adjust better, but it still looks quite wonky. Also because it is inline, we don't need it at all. The spacing would probably even work better without it because it would act like an image and just align automatically.

The flexbox takes care of two things here:

aligning the icon at the vertical center even when the link is two lines (popularity plugin)

move the icon to the right when using RTL

The icon automatically goes to the right when using RTL because it is inline-block by default. You don't need flexbox for that. It would make more sense to use flexbox for the two uls for that reason than the icon. (Although I don't think it would be worth it as the extra CSS for RTL is only one line.)

I personally don't like the vertically centred icon on items with two lines. It disturbs the horizontal rhythm. But in both cases the current wonkiness improves by adding vertical-align: bottom to the svg.

selfthinker · 2017-01-29T20:11:59Z

lib/tpl/dokuwiki/style.ini

@@ -31,7 +31,7 @@ css/_diff.css             = screen
 css/_edit.css             = screen
 css/_modal.css            = screen
 css/_forms.css            = screen
-css/_admin.css            = screen
+css/_admin.less           = screen


Renaming this would break a few templates which import the file directly in their style.ini. Not the end of the world, but just something we need to consider.

selfthinker · 2017-01-29T20:12:45Z

lib/tpl/dokuwiki/css/_admin.css

-}
-
-.dokuwiki ul.admin_tasks li.admin_acl {
-    background-image: url(../../images/admin/acl.png);


These images should be deprecated instead of removing them. Some templates still use them.

selfthinker · 2017-01-29T20:17:20Z

I added an admin.svg to an admin plugin and it was not showing. I purged the cache and also looked into my error log but nothing related was in there. Any idea how to debug?

splitbrain · 2017-01-30T09:50:54Z

Check the file size. Your image has to be smaller than 2kb

We could add info about this to the debug log.

selfthinker · 2017-01-30T11:00:25Z

The file size is 573 bytes. So that can't be it.

splitbrain · 2017-01-30T12:47:30Z

can you attach the file here?

selfthinker · 2017-01-30T23:15:19Z

I doubt that it's the file, I would think it's more likely to do with me using PHP on Windows (although nothing shows in my error log). Although it makes sense to check the file as well, I cannot attach it here as it's an SVG and GitHub doesn't support that. I'll paste the content instead:

<?xml version="1.0" encoding="utf-8"?>
<!-- Generated by IcoMoon.io -->
<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
<svg version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="64" height="64" viewBox="0 0 64 64">
<path d="M64 20l-32-16-32 16 32 16 32-16zM32 9.311l21.379 10.689-21.379 10.689-21.379-10.689 21.379-10.689zM57.59 28.795l6.41 3.205-32 16-32-16 6.41-3.205 25.59 12.795zM57.59 40.795l6.41 3.205-32 16-32-16 6.41-3.205 25.59 12.795z" fill="#000000"></path>
</svg>

splitbrain · 2017-01-31T17:12:11Z

Actually it is the file. I'll push an update soon.

now comments and line breaks between tags are removed

This reverts commit b8b60fd. The images are now to be seen as deprecated but will remain for a little longer. They will be released in the future.

* use inline-block instead of flexbox * fix RTL alignments

splitbrain · 2017-01-31T18:16:15Z

I updated the styles to make use of inline-block instead of flexbox and fixed the RTL styles. Icons are now top aligned. It's as good as I can make it. Feel free to add your own adjustments. I'll probably merge it this weekend.

micgro42 · 2017-02-03T11:48:18Z

Works as expected.

~~Maybe I should have tested this earlier, but it currently looks rather broken:~~

~~I see this both on Firefox 51.0.1 and Chromium 56.0.2924.76~~

splitbrain · 2017-02-03T11:49:17Z

What template? Did you touch your config for a cache refresh?

splitbrain · 2017-02-03T11:53:24Z

if the problem remains, please open a new issue, so we can properly track it without reopening the PR

micgro42 · 2017-02-03T11:59:18Z

I still used my slightly adjusted copy of the dokuwiki template from before the merge. With the actual dokuwiki template it works as expected. Sorry for the noise.

ThisNameIsNotAllowed and others added 7 commits January 21, 2017 11:24

Use inline SVG for Admin Plugin icons and style them via CSS

3c27983

This introduces an embedSVG() function that can be used at other places.

increased the default limit for embedded SVGs to 2kb

1935f90

more complex material design icons can be somewhat larger than 1kb

added SVG icons for our admin actions

47d645c

using icons from https://materialdesignicons.com/

Adds proper styling for new Admin UI

713faa9

removed the no longer used admin PNG icons

b8b60fd

splitbrain added Category: Design ToDo: QuickMerge labels Jan 21, 2017

splitbrain added this to the Frusterick Manners milestone Jan 21, 2017

splitbrain requested review from selfthinker, Klap-in and michitux January 21, 2017 13:35

splitbrain mentioned this pull request Jan 21, 2017

Provide icons for plugins in admin menu #1767

Closed

splitbrain added the ToDo: needDocUpdate label Jan 21, 2017

splitbrain added 2 commits January 21, 2017 14:46

old PHP versions can't calculate in the initialzing

ebf33c9

fixed access to wrong variable

eb8a855

selfthinker reviewed Jan 29, 2017

View reviewed changes

splitbrain added 5 commits January 31, 2017 18:29

more cleanup in embedSVG

0849fa8

now comments and line breaks between tags are removed

Revert "removed the no longer used admin PNG icons"

6dfd365

This reverts commit b8b60fd. The images are now to be seen as deprecated but will remain for a little longer. They will be released in the future.

improve admin styling

220b8a2

* use inline-block instead of flexbox * fix RTL alignments

use lower case class names

5d2e38c

removed unneeded clearer

d1a3148

typo fix

ad4b324

splitbrain merged commit aa1f0d9 into master Feb 3, 2017

splitbrain deleted the adminicons branch February 3, 2017 08:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Admin Page refactoring #1809

Admin Page refactoring #1809

splitbrain commented Jan 21, 2017 •

edited by selfthinker

mention-bot commented Jan 21, 2017

selfthinker Jan 29, 2017

splitbrain Jan 31, 2017

selfthinker Jan 29, 2017

selfthinker Jan 29, 2017

selfthinker Jan 29, 2017

splitbrain Jan 31, 2017

selfthinker Jan 29, 2017

splitbrain Jan 30, 2017

selfthinker Jan 30, 2017

splitbrain Jan 30, 2017

selfthinker Jan 30, 2017

selfthinker Jan 29, 2017

splitbrain Jan 30, 2017

splitbrain Jan 31, 2017

selfthinker Jan 29, 2017 •

edited

splitbrain Jan 30, 2017

selfthinker Jan 30, 2017

selfthinker Jan 29, 2017

selfthinker Jan 29, 2017

selfthinker commented Jan 29, 2017

splitbrain commented Jan 30, 2017 •

edited

selfthinker commented Jan 30, 2017

splitbrain commented Jan 30, 2017

selfthinker commented Jan 30, 2017

splitbrain commented Jan 31, 2017

splitbrain commented Jan 31, 2017

micgro42 commented Feb 3, 2017 •

edited

splitbrain commented Feb 3, 2017

splitbrain commented Feb 3, 2017

micgro42 commented Feb 3, 2017

Admin Page refactoring #1809

Admin Page refactoring #1809

Conversation

splitbrain commented Jan 21, 2017 • edited by selfthinker

mention-bot commented Jan 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

selfthinker Jan 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

selfthinker commented Jan 29, 2017

splitbrain commented Jan 30, 2017 • edited

selfthinker commented Jan 30, 2017

splitbrain commented Jan 30, 2017

selfthinker commented Jan 30, 2017

splitbrain commented Jan 31, 2017

splitbrain commented Jan 31, 2017

micgro42 commented Feb 3, 2017 • edited

splitbrain commented Feb 3, 2017

splitbrain commented Feb 3, 2017

micgro42 commented Feb 3, 2017

splitbrain commented Jan 21, 2017 •

edited by selfthinker

selfthinker Jan 29, 2017 •

edited

splitbrain commented Jan 30, 2017 •

edited

micgro42 commented Feb 3, 2017 •

edited