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.

Sign up for GitHub

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

Jump to bottom

Theme manager: improvements #732

Merged
merged 4 commits into from
Jan 6, 2017
Merged

Theme manager: improvements #732

merged 4 commits into from
Jan 6, 2017

Conversation

ArthurHoaro
Copy link
Member

Speeding things up a bit regarding #705:

  • add a method to the Updater to keep custom theme settings
  • get theme function moved to ApplicationUtils
  • inc/ folder in theme directory renamed to css/
  • 3rd party themes git ignored
  • unit tests

fixes #22 #502
closes #705

ping @Knah-Tsaeb

@ArthurHoaro ArthurHoaro added this to the 0.9.0 milestone Jan 3, 2017
@ArthurHoaro ArthurHoaro self-assigned this Jan 3, 2017
virtualtam
virtualtam requested changes Jan 3, 2017
View reviewed changes
tpl/default/configure.html
{loop="$theme_available"}
<option value="{$value}"
{if="$value===$theme"}
selected="selected"
Copy link
Member

Choose a reason for hiding this comment

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

I think selected takes no value (else W3C checkers raise an error):

<option value="toto" selected>Toto</option>

Copy link
Member Author

Choose a reason for hiding this comment

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

Apparently, both are valid. But I thought that only selected="selected" was.

COPYING
@@ -43,7 +43,7 @@ License: CC-BY (http://creativecommons.org/licenses/by/3.0/)
Copyright: (c) 2014 Designmodo
Source: http://designmodo.com/linecons-free/

Files: images/floral_left.png, images/floral_right.png, images/squiggle.png, images/squiggle2.png, images/squiggle_closing.png
Files: images/floral_left.png, images/floral_right.png, images/squiggle.png, images/squiggle_closing.png
Copy link
Member

@virtualtam virtualtam Jan 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

not related to this PR, but there seems to be a regression regarding the positioning of the squiggle on the Daily page: it used to be on the right and is now displayed on the left

Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't find any breaking change recently, it may have changed a long time ago. I'll fix it.

application/ApplicationUtils.php
*
* @return array List of theme names.
*/
public static function getThemes($tplDir)
Copy link
Member

Choose a reason for hiding this comment

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

would it make sense to create a ThemeUtils class to encapsulate theme operations?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, why not.

tests/ApplicationUtilsTest.php
*/
public function testGetThemesEmpty()
{
mkdir('sandbox/tpl/', 0777, true);
Copy link
Member

Choose a reason for hiding this comment

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

unsafe: this will leave a world-writable folder in case the tests fail

suggestions:

  • create the folder in setUp() or setUpClass and remove it in tearDown or tearDownClass
  • 0755 should be sufficient, theme directories do not need to be writable by the HTTP server user/group

tests/ApplicationUtilsTest.php
@@ -328,4 +331,48 @@ public function testCheckCurrentResourcePermissionsErrors()
ApplicationUtils::checkResourcePermissions($conf)
);
}

Copy link
Member

Choose a reason for hiding this comment

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

unsafe: this will leave world-writable folders in case the tests fail

suggestions:

  • create a parent folder in setUp() or setUpClass and remove it in tearDown or tearDownClass
  • 0755 should be sufficient, theme directories do not need to be writable by the HTTP server user/group

@ArthurHoaro ArthurHoaro force-pushed the feature/theme-manager branch from b50d341 to cadbc84 Compare January 5, 2017 11:03
@ArthurHoaro ArthurHoaro force-pushed the feature/theme-manager branch from cadbc84 to 508e398 Compare January 5, 2017 11:05
@ArthurHoaro
Copy link
Member Author

Updated and rebased!

virtualtam
virtualtam requested changes Jan 5, 2017
View reviewed changes
index.php
@@ -1151,6 +1153,8 @@ function renderPage($conf, $pluginManager, $LINKSDB)
else // Show the configuration form.
{
$PAGE->assign('title', $conf->get('general.title'));
$PAGE->assign('theme', $conf->get('resource.theme'));
$PAGE->assign('theme_available', ApplicationUtils::getThemes($conf->get('resource.raintpl_tpl')));
Copy link
Member

@virtualtam virtualtam Jan 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Shaarli\ThemeUtils::getThemes(...)

Copy link
Member Author

Choose a reason for hiding this comment

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

Caught not testing :o

@ArthurHoaro ArthurHoaro force-pushed the feature/theme-manager branch from 508e398 to 01c6e32 Compare January 5, 2017 15:16
@virtualtam virtualtam mentioned this pull request Jan 5, 2017
Closed
20 tasks
virtualtam
virtualtam approved these changes Jan 5, 2017
View reviewed changes
Copy link
Member

@virtualtam virtualtam left a comment

Choose a reason for hiding this comment

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

Tested with @kalvn's Material theme

@alexisju
Copy link

alexisju commented Jan 5, 2017

How to make theme compliant?

@ArthurHoaro
Copy link
Member Author

@alexisju if your theme is supposed to be a subdirectory in tpl/, not much, just this part..

@alexisju
Copy link

alexisju commented Jan 5, 2017

Ok. So no need meta file or standardised CSS for now?

@ArthurHoaro
Copy link
Member Author

Nope.

@ArthurHoaro ArthurHoaro merged commit 7418f7c into shaarli:master Jan 6, 2017
@ArthurHoaro ArthurHoaro deleted the feature/theme-manager branch January 6, 2017 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@virtualtam virtualtam virtualtam approved these changes

Assignees

@ArthurHoaro ArthurHoaro

Labels
enhancement in review template HTML rendering
Projects
None yet
Milestone
0.9.0
Development

Successfully merging this pull request may close these issues.

Add themes management
4 participants
@ArthurHoaro @alexisju @virtualtam @Knah-Tsaeb