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

First simple implementation to allow to set custom theming of yunohost. #631

Open
wants to merge 2 commits into
base: stretch-unstable
from

Conversation

Projects
None yet
2 participants
@eauchat
Copy link

eauchat commented Jan 28, 2019

The problem

YNH's SSO look is not so straightforwardly customizable.

Solution

Following on issue YunoHost/issues#1267, I was trying to bring the possibility to add custom themes.
For now it's just a simple CLI interface that allow to choose the currently active theme.
Installing the themes is still to be done copying files manually for now.

How to test

  • Get the changes from pull request YunoHost/SSOwat#116.
  • ( Run yunohost themes list you should see "clouds", and "black". )
  • Run yunohost themes set clouds, this will set the current theme to clouds, go check it out visiting the SSO in the browser.

You can create any custom theme by manually creating a directory in /usr/share/ssowat/portal/assets/themes/ and adding the necessary css files to it (css/ynhpanel.css and css/ynh-style.css).

Next steps

  • a command to create a blank theme (that create the directory and necessary css files)
  • a command to unset custom theme (switch back to default mode)
  • create a package manager for pre-made themes that could be easily fetched and installed from any chosen repo. Something similar to what exist for apps.

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :
@eauchat

This comment has been minimized.

Copy link
Author

eauchat commented Jan 28, 2019

One thing that's missing in this pull request, is how to make sure that the "themes" directory is not removed on yunohost upgrades. But I don't know how and where that should be set up.

@eauchat

This comment has been minimized.

Copy link
Author

eauchat commented Jan 28, 2019

I just realized that /etc/ssowat/conf.json is automatically refreshed by the function "app_ssowatconf" on many admin actions. It seems that the way I implemented the saving of the theme name is not good then, I guess it should use this function. However I don't really understand where would be good to save it so that app_ssowatconf could fetch it.

@alexAubin

This comment has been minimized.

Copy link
Member

alexAubin commented Jan 28, 2019

Uh yes indeed ...

Well I'm really wondering if we shouldn't use the setting mechanism to handle this. This list / get / set interface look a lot like what's already available in it : https://github.com/YunoHost/yunohost/blob/stretch-unstable/src/yunohost/settings.py

We could have directly something like yunohost settings [set|get] ssowat.theme or something like this.

The thing is that we're still missing a mechanism that actually triggers action/changes when we change a setting value... One thing we thought about was to use decorator to have something like this :

@onchange("ssowat.theme")
def update_ssowat_conf():
    app_ssowatconf()

and this onchange decorator thing would make it so that each time a setting is changed, the corresponding functions are called. (If you already know about decorators, what I just wrote will feel trivial, but it you don't it will feel mystic x))

@eauchat

This comment has been minimized.

Copy link
Author

eauchat commented Jan 28, 2019

Yes, actually I realize that using settings could be much simpler than implementing the new themes interface. The thing is that if the themes purpose is to let install custom themes like a package manager, the settings interface won't be adapted for that.

I think I can understand the concept of decorator, but from very far ;/ I'm really discovering python, so I don't feel competent to implement such behaviour.

I don't know if that's over-complicated, but I can imagine that the themes (get/set/list) interface could trigger yunohost settings [set|get] ssowat.theme. And on set, also trigger app_ssowatconf for refreshing "/etc/ssowat/conf.json".
That could be a temporary fix before the decorator thing is implemented (it's just a hack, because if someone decided to use the settings interface to set the theme, they wouldn't understand why it doesn't take effect and that's no good).

@eauchat

This comment has been minimized.

Copy link
Author

eauchat commented Jan 28, 2019

I don't know, I think the specific theme CLI interface is cool with the "package manager" idea, but I don't know if anyone also likes that. It's one more option at the top level of yunohost CLI, and if no one is so interested in it, it might not make sense to create that.
It might just be my special attachment to wanting users* to easily customize the world they are living in.

*Admins in this case, but they are users of the yunohost distribution.

@eauchat

This comment has been minimized.

Copy link
Author

eauchat commented Jan 29, 2019

Ok, with the last commit, the theme's value persists, it's properly gotten from settings when the ssowat config is refreshed.

Using settings made it simple to save the value of the current theme.
However, at the moment, using yunohost settings set "ssowat.theme" -v someThemeName shouldn't be allowed, because it doesn't properly check if the asked theme is available, and it also doesn't refresh ssowat config.

Maybe it would be useful to implement a way to do those checks from yunohost settings set but it seems a bit complex no?
Or implement a new parameter like: ("ssowat.theme", {"type": "string", "default": "default", "customSet": "this setting must be set with 'yunohost theme set' interface" }).
customSet if defined would disable the possibility to set a setting from yunohost settings interface and log the given error message if one tries to.
But maybe that's also unnecessarily confusing for users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment