Themes #162

2018-12-22T17:35:06Z

laurensmartina commented

2018-12-22 17:35:06 +00:00

(Migrated from github.com)

Initial introduction of themes.

Partial solution for #23.

Initial introduction of themes. Partial solution for #23.

matthijskooijman (Migrated from github.com) reviewed

2018-12-29 11:24:59 +00:00

matthijskooijman (Migrated from github.com) left a comment

Code looks ok, but I'm not entirely convinced this is heading in the right direction.

Ideally, I'd like to see no git-tracked files in the data directory, and let all files that need to be written to live in data, to simplify permission handling.
IIRC we talked about themes, but also talked about some ways to modify the CSS and HTML in an extensible way through the webinterface. I somewhat recall that the latter would be simpler to use and might actually reduce the need for themes (and perhaps themes would even unnecesarily complicate this editor). OTOH, multiple switchable themes would make things easier for ourselves during development, of course...

Probably something to discuss this afternoon.

Code looks ok, but I'm not entirely convinced this is heading in the right direction. - Ideally, I'd like to see *no* git-tracked files in the `data` directory, and let all files that need to be written to live in `data`, to simplify permission handling. - IIRC we talked about themes, but also talked about some ways to modify the CSS and HTML in an extensible way through the webinterface. I somewhat recall that the latter would be simpler to use and might actually reduce the need for themes (and perhaps themes would even unnecesarily complicate this editor). OTOH, multiple switchable themes would make things easier for ourselves during development, of course... Probably something to discuss this afternoon.

system/core/base.php

What is the meaning of $src here? Also, couldn't we just do something like Hypha::$data->css->url_path or something?

What is the meaning of `$src` here? Also, couldn't we just do something like `Hypha::$data->css->url_path` or something?

matthijskooijman (Migrated from github.com) reviewed

2019-01-02 18:29:52 +00:00

matthijskooijman (Migrated from github.com) left a comment

I left a few more comments inline. I'm still missing the "new theme" option, but it would be fine to add that later. Now that you can edit the current theme, I think this PR should be complete and (nearly) mergeable (after resolving my comments).

index.php

I suspect that this (and probably in the previous commit already) should allow serving of all themes, regardless of the selected theme. This makes the check simpler, but also allows one theme to include another (e.g. have a theme that overrides stuff in the default theme rather than replacing it entirely).

I suspect that this (and probably in the previous commit already) should allow serving of *all* themes, regardless of the selected theme. This makes the check simpler, but also allows one theme to include another (e.g. have a theme that overrides stuff in the default theme rather than replacing it entirely).

index.php

					
				@ -68,3 +67,2 @@

					if (strpos($hyphaQuery, 'data/css/') === 0) serveFile($hyphaQuery, false);

					if (strpos($hyphaQuery, 'data/fonts/') === 0) serveFile($hyphaQuery, false);

					if (startsWith($hyphaQuery, 'data/themes/')) serveFile($hyphaQuery, 'data/themes');

					if (startsWith($hyphaQuery, 'system/wymeditor')) serveFile($hyphaQuery, 'system/wymeditor');

Did you intentionally remove support for fonts here? Or is the idea to put these inside the themes directory?

system/datatypes/settings.php

I would recommend using htmlspecialchars here, since $theme can (and should be allowed to) contain any characaters.

I would recommend using `htmlspecialchars` here, since `$theme` can (and should be allowed to) contain any characaters.

tammoterhark (Migrated from github.com) reviewed

2019-01-04 14:40:22 +00:00

system/core/base.php

I second you on having no git-tracked files in data. Maybe the default theme should reside somewhere in core and be not-editable.

Yes, in due time there should be a mechanism to copy the default theme to the theme dir and work from there. But since this partial solution already would lighten the work for working on themes, I would propose this go ahead.

I second you on having no git-tracked files in data. Maybe the default theme should reside somewhere in core and be not-editable. Yes, in due time there should be a mechanism to copy the default theme to the theme dir and work from there. But since this partial solution already would lighten the work for working on themes, I would propose this go ahead.

tammoterhark (Migrated from github.com) reviewed

2019-01-04 14:41:46 +00:00

index.php

					
				@ -68,3 +67,2 @@

					if (strpos($hyphaQuery, 'data/css/') === 0) serveFile($hyphaQuery, false);

					if (strpos($hyphaQuery, 'data/fonts/') === 0) serveFile($hyphaQuery, false);

					if (startsWith($hyphaQuery, 'data/themes/')) serveFile($hyphaQuery, 'data/themes');

					if (startsWith($hyphaQuery, 'system/wymeditor')) serveFile($hyphaQuery, 'system/wymeditor');

If fonts are outside of the theme directory, it will be more difficult to transfer a theme from one installation to another.

Althought this may lead to some duplification of font files, I think the ease of installing would win here.

If fonts are outside of the theme directory, it will be more difficult to transfer a theme from one installation to another. Althought this may lead to some duplification of font files, I think the ease of installing would win here.

matthijskooijman (Migrated from github.com) reviewed

2019-01-05 10:22:32 +00:00

index.php

					
				@ -68,3 +67,2 @@

					if (strpos($hyphaQuery, 'data/css/') === 0) serveFile($hyphaQuery, false);

					if (strpos($hyphaQuery, 'data/fonts/') === 0) serveFile($hyphaQuery, false);

					if (startsWith($hyphaQuery, 'data/themes/')) serveFile($hyphaQuery, 'data/themes');

					if (startsWith($hyphaQuery, 'system/wymeditor')) serveFile($hyphaQuery, 'system/wymeditor');

Sounds good, let's indeed keep fonts inside of the theme. I'll add a note to the commit message.

matthijskooijman (Migrated from github.com) reviewed

2019-01-05 10:22:42 +00:00

system/datatypes/settings.php

I just fixed this locally.

matthijskooijman (Migrated from github.com) reviewed

2019-01-05 10:38:51 +00:00

index.php

Fixed locally.

matthijskooijman commented

2019-01-05 11:06:06 +00:00

(Migrated from github.com)

I also noticed that hypha.html was converted from dos to unix line endings. Now that the default theme is no longer intended to be edited, this makes sense, but it should be in a separate commit. I refrained from this for now, to prevent conflicts with #172.

I also added a "cancel" button to the theme switch settings dialog.

Finally, I cleaned up a few other things. I just pushed these changes to this PR.

I also noticed that hypha.html was converted from dos to unix line endings. Now that the default theme is no longer intended to be edited, this makes sense, but it should be in a separate commit. I refrained from this for now, to prevent conflicts with #172. I also added a "cancel" button to the theme switch settings dialog. Finally, I cleaned up a few other things. I just pushed these changes to this PR.

matthijskooijman commented

2019-01-05 11:44:32 +00:00

(Migrated from github.com)

I added #175 for some additional improvements on this PR.

Sign in to join this conversation.

No reviewers

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

harmen/hypha!162

Rows
Columns