-
Notifications
You must be signed in to change notification settings - Fork 291
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
#502 Change templates set through administration UI #705
Conversation
Huuuuu, test fail : ( |
Hi! Thanks for your contribution! A few general remarks though:
|
Why not ? I don't find a good reason for personalise this setting. But why not.
Me not ;-), I consider a name of template like proper name like title of book or music. Is a personal preference and vision, not a rule or law.
Sorry I not precise for CSS, yes they move into tpl folder, and the user.css keep in inc dir for non breaking existing user personalisation.
Ok my bad, I undo this. I try to fix test tomorrow, now is children time. |
Someone might be already using it, and there is no apparent reason to remove this feature.
Maybe, but I was talking about the folder name: all folders are in lower case in Shaarli (well, except in tests). That's not very important, but I'd like to keep consistency. Good luck with the tests, feel free to ask if you need help. |
Ok, one test fail on my machine, but work on Travis, yeah \o/ |
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.
Hi! I've reviewed your PR and made a bunch of comments to improve it. Nothing major, but it requires attention.
@@ -1173,6 +1175,8 @@ function renderPage($conf, $pluginManager) | |||
else // Show the configuration form. | |||
{ | |||
$PAGE->assign('title', $conf->get('general.title')); | |||
$PAGE->assign('theme', $conf->get('resource.theme')); | |||
$PAGE->assign('theme_avaible', getAllTheme()); |
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.
s/avaible/available/
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.
Yep sorry, I fix it soon.
@@ -28,9 +28,9 @@ | |||
</div> | |||
|
|||
<div class="dailyTitle"> | |||
<img src="../images/floral_left.png" width="51" height="50" class="nomobile" alt="floral_left"> | |||
<img src="../../images/floral_left.png" width="51" height="50" class="nomobile" alt="floral_left"> |
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.
In RainTPL you can use src="images/floral_left.png#"
, the #
meaning it will create a path from the root directory.
But aren't these image theme related?
<link type="text/css" rel="stylesheet" href="../inc/reset.css" /> | ||
<link type="text/css" rel="stylesheet" href="../inc/shaarli.css" /> | ||
{if="is_file('inc/user.css')"}<link type="text/css" rel="stylesheet" href="../inc/user.css" />{/if} | ||
<link type="text/css" rel="stylesheet" href="reset.css" /> |
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.
A CSS subfolder would be cleaner.
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.
Same for awesomplete lib and blazy lib JS ? And awesomplete CSS ?
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.
Hmm... no. While it's true that it linked to the theme, custom theme might also use them. So I'd say that we keep them in inc/
.
@@ -1,7 +1,7 @@ | |||
<!DOCTYPE html> | |||
<html> | |||
<head> | |||
<link type="text/css" rel="stylesheet" href="../inc/awesomplete.css" /> | |||
<link type="text/css" rel="stylesheet" href="../../inc/awesomplete.css" /> |
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.
Same comment regarding relative path.
@@ -846,7 +846,7 @@ div.dailyEntryThumbnail { | |||
width: 100%; | |||
text-align: center; | |||
background-color: rgb(128, 128, 128); | |||
background: url(../images/50pc_transparent.png); | |||
background: url(../../images/50pc_transparent.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.
All these images should go under tpl/default/images
, except, eventually, the logo.
|
||
function getAllTheme() | ||
{ | ||
$allTheme = glob('tpl/*', GLOB_ONLYDIR); |
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.
pass the resource.raintpl_tpl
parameter to the function, and use it.
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.
Yes I forgot this passage.
@@ -79,7 +79,7 @@ private function initialize() | |||
$this->tpl->assign('hide_timestamps', $this->conf->get('privacy.hide_timestamps', false)); | |||
$this->tpl->assign('token', getToken($this->conf)); | |||
// To be removed with a proper theme configuration. | |||
$this->tpl->assign('conf', $this->conf); | |||
$this->tpl->assign('conf', $this->conf->get('resource.theme', 'default')); |
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.
My comment isn't clear here, but this isn't set just to pass the theme name. The whole configuration manager is passed in conf
variable because themes can use custom settings and need to access them.
Add this instead:
$this->tpl->assign('theme', $this->conf->get('resource.theme', 'default'));
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 understand but OK.
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.
Sorry, I made a mistake. You assign the theme
variable in index.php
. You can actually let this file untouched (keep assign('conf', $this->conf);
).
@@ -1148,6 +1149,7 @@ function renderPage($conf, $pluginManager) | |||
$conf->set('general.timezone', $tz); | |||
$conf->set('general.title', escape($_POST['title'])); | |||
$conf->set('general.header_link', escape($_POST['titleLink'])); | |||
$conf->set('resource.theme', escape($_POST['theme'])); |
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.
It might be a good idea to delete the page cache after writing this conf:
invalidateCaches($conf->get('resource.page_cache'));
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.
That work without clearing cache. But I can add it.
…tests, etc.)" This reverts commit f78c9ee.
…tests, etc.)" This reverts commit f78c9ee.
Hi,
I make a theme changer refer #502 and #22.
In this change,
Have a nice day !