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

Template Style Dialog Plugin #1163

Merged
merged 38 commits into from Jul 26, 2015
Merged

Template Style Dialog Plugin #1163

merged 38 commits into from Jul 26, 2015

Conversation

splitbrain
Copy link
Collaborator

This plugin allows administrators to adjust the style.ini settings. Works fine for me but should be tested by others for usability.

@chang-zhao
Copy link
Contributor

1: The doc page https://www.dokuwiki.org/plugin:styler is about another plugin called styler (2007).

2: "Template Style Settings" popover appears to me empty and partly off-screen, and jumps down when I try to resize it. Firefox 31.

@splitbrain
Copy link
Collaborator Author

Ah I guess we have to find a different name then.

Are you sure you're running a fresh cache? Try touching conf/local.php. Can you provide any error messages?

@chang-zhao
Copy link
Contributor

It seems the problem is here:
jQuery.post(
DOKU_BASE + '/lib/exe/ajax.php',
...
$dialog.load(
DOKU_BASE + '/lib/exe/ajax.php',

Because DOKU_BASE = '/'
the call goes to http://lib/exe/ajax.php

@chang-zhao
Copy link
Contributor

Yes, removing first slash from '/lib/exe/ajax.php' helps.
The rest:

  • When JS works, it makes double && in the address:
    http://test/doku.php?id=start&do=admin&page=styler
    =>
    http://test/doku.php?id=start&&page=styler
    • Popover positioning is strange. Initially its top is behind my upper screen border; when I try to resize it it jumps to the bottom of my screen (though the position at the bottom is probably intended (?) - not interfering with a usual page layout).
    • (Also I don't see "Preview your changes" button in a popover, but that's probably intended).
    • In general, color picker and so on seems OK to me, at least when using a mouse.
      Using keyboard might cause some inconveniencies with scrolling to the GUI of the current color picker.

@splitbrain
Copy link
Collaborator Author

good catches. I fixed them.

Regarding the positioning. That's weird. It's supposed to be in the lower left corner. What template do you use and what browsers?

This commit consists of patches automatically generated for this project on https://scrutinizer-ci.com
@chang-zhao
Copy link
Contributor

Standard template, Firefox 31.
Firebug inspection shows positioning with some negative numbers in "top", for example:

<div class="ui-dialog ui-widget ui-widget-content ui-corner-all ui-front ui-draggable ui-resizable" style="position: relative; height: auto; width: 500px; top: -706.567px; left: -8px; display: block;" tabindex="-1" role="dialog" aria-describedby="ui-id-1" aria-labelledby="ui-id-2">

I use zooming FF plugins such as NoSquint, but maybe the problem is somewhere else.

@@ -47,6 +47,7 @@
!/lib/plugins/popularity
!/lib/plugins/revert
!/lib/plugins/safefnrecode
!/lib/plugins/styler
Copy link
Collaborator

Choose a reason for hiding this comment

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

You forgot to swap the name here as well.

I personally don't like either "styler" or "styling". Those names are far too generic and sound like they would do something else. What was wrong with its original name "templatestyler"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The name is in spirit with "config" and "extension". There was never a "original name". I started with styler which was taken.

Copy link
Collaborator

Choose a reason for hiding this comment

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

With "original name" I mean the one I gave the prototype which you based this on. The folder was called "templatestyler".

The problem I have with calling it "styling" is that it's not clear what it is styling. And it is not even "styling" a lot. It is changing existing styles based on a handful of variables.
It is not the same as "config" or "extension".

@splitbrain
Copy link
Collaborator Author

@selfthinker @chang-zhao can you have a look at this again? I completely removed the jQuery UI dialog in favor of a real popup (new browser window). This simplifies the whole handling a lot and has the advantage that we can display the popup with a sane style even if a user (temporarily) breaks the styles.

I reintroduced the preview button because preview is relatively slow. A simple loading screen takes care of informing the user that something is happeing.

Since I no longer use the cookie, a user needs to press "preview" whenever she browses to a different page. Is that okay or should I reintroduce the cookie?

@chang-zhao
Copy link
Contributor

If there is a reason to not use cookie, then it's OK. We just give a user warning about that, and (s)he presses "Preview" each time. Shouldn't be a big problem IMHO.

There is still that thing with Color pickers: if you focus on a numeric input field, located near the bottom of a viewport, then a picker panel can appear (in most part) below the bottom of the window. Then choosing a color by arrow keys etc can happen in the invisible part.
Don't know, maybe keyboard users know well how to handle such situations. (I use mouse). At least it's possible to Tab to next fields and then Shift-Tab back. Though it might be clumsy. So maybe it's cool to scroll a bit via Javascript when a picker panel opens, to get it all in a window.

Generally I'm very happy with the plugin, it is a very good solution to an important problem.

Yes, generally, OK.

@selfthinker
Copy link
Collaborator

Sorry, I couldn't find the time to look at those changes yet. I will have time at the end of next week at the latest.

if (!$styling_plugin.hasClass('ispopup')) {
var $hl = $styling_plugin.find('h1').first();
var $btn = jQuery('<button class="btn">' + LANG.plugins.styling.popup + '</button>');
$hl.append($btn);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is the button part of the h1?
I think it would make more sense after the intro paragraph (or before the form as that is easier to hook into).

* improved spacing of popup
* made primary buttons clearer
* xhtml and validity fixes
* improved some lang strings
* moved 'open as popup' after intro
* fixed page reload after clicking 'open as popup' button
@selfthinker
Copy link
Collaborator

I have fixed a few minor things.
I also looked into why a previously opened color picker wouldn't close. That seems to be how Iris works and they don't provide a way to overwrite this. So, there's nothing we can do, and it's not very important anyway.

I have tested this but not extensively. From my point of view this is good to go. 👍

Re: cookie and preview. I would prefer to have the cookie back and be able to browse the wiki with a persistent preview style. But that is not a must-have and can be done in a separate PR.

The icon does not match the others because the NuvolaX icon theme seems
to have vanished from the Internet. Ideally all those admin icons could
use a replacement. But that should be a different PR I guess.
@splitbrain
Copy link
Collaborator Author

I readded the cookie and moved the plugin to the bundled plugins. The icon sucks but is the best I could come up with right now.

@selfthinker good to merge?

@selfthinker
Copy link
Collaborator

Further testing...

I just opened my test wiki in IE 11, am not logged in and can see the changes plus the preview message with every reload. I can even see those when the other browser has closed the admin section and the styling popup.
As I had tested the plugin before, I think this is because the cookie is still set. Playing around further, i.e. opening and closing the styling admin section (i.e. unsetting the cookie) "fixes" it.

Testing the plugin itself in IE 11 shows that the popup is not working at all (no CSS is loading and when you navigate to a different page and "preview changes" the CSS is gone again). But there are JS errors:

SCRIPT5022: SyntaxError
File: js.php, Line: 36701, Column: 13

That is target.jQuery('body').append($loader);

Quickly tested in IE 8, that seems to have very similar issues.

From playing around in Chrome and Firefox, I only found that the caching should be improved. When one browser saves the settings (or reverts to template's default), the other only sees them after a hard refresh.

IE doesn't like it when you create a DOM element in one window and
try to insert it in another window.
@splitbrain
Copy link
Collaborator Author

Okay IE problem should be fixed. I also added the style.ini to the things that makes the browser recheck for fresh css.

@selfthinker
Copy link
Collaborator

This is still not working for me in IE11. When I open the popup, the page in the background doesn't load the CSS and is only showing the "Preview is loading" message which doesn't go away. But I don't get an error message this time.
Weirdly enough, it is working in IE8.

And master needs to be merged into this branch.

@selfthinker
Copy link
Collaborator

Merged master into this and fixed the merge conflict.

hopefully doesn't break in the other browsers
@scrutinizer-notifier
Copy link

The inspection completed: 1 new issues, 18 updated code elements

@selfthinker
Copy link
Collaborator

Tested in IE11, 10, 9 and 8 and all working fine now. :)
👍

selfthinker added a commit that referenced this pull request Jul 26, 2015
Template Style Dialog Plugin
@selfthinker selfthinker merged commit 6439cee into master Jul 26, 2015
@selfthinker selfthinker deleted the styler branch July 26, 2015 14:06
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.

None yet

6 participants