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

Some example themes to test #118

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

Conversation

Projects
None yet
3 participants
@eauchat
Copy link

eauchat commented Feb 1, 2019

This PR just contains all changes from #117 plus some basic example themes to easily test the new "yunohost themes" API.

@@ -34,10 +37,6 @@
</head>
<body class="{{#connected}}logged{{/connected}}">

<h1 id="logo" class="logo">
<img src="assets/img/logo-ynh-white.svg"/><span class="element-invisible">Yunohost</span>
</h1>

This comment has been minimized.

@alexAubin

alexAubin Feb 21, 2019

Member

Why remove this tho ? D:

This comment has been minimized.

@eauchat

eauchat Feb 21, 2019

Author

Hey, yes we discussed a bit about this in #121 with @lfuelling.
I had chosen to remove the logo here because it was confusing to have a logo that's clickable in the panel and not clickable in the homepage.

In the end, IMHO the best option would be to have a logo that's always there and a button that's clickable, so yes, maybe not remove those lines (and also the styles necessary for it).

This comment has been minimized.

@lfuelling

lfuelling Feb 21, 2019

Contributor

#121 reverts this change and also fixes another issue with the overlay (see my review in #117).

My approach to the logo would be to disable it in a theme when it's not wanted, but maybe this also could be done using configuration (a bit overkill imo).


// Get user's infos
var r = new XMLHttpRequest();
r.open("GET", "/ynhpanel.json", true);

This comment has been minimized.

@alexAubin

alexAubin Feb 21, 2019

Member

Eh, where is that supposed to come from ? x_x

This comment has been minimized.

@alexAubin

alexAubin Feb 21, 2019

Member

Hm now that I dig a bit deeper I kinda understand, but while testing on my machine, before logging in, it can't access this file, and after logging in there's no 'theme' property yet defined apparently :s

This comment has been minimized.

@eauchat

eauchat Feb 21, 2019

Author

Well this comes initially from the ynhpanel.js file, it was used to fetch config and is not necessary there anymore. I can't say I really understand the organization behind the serving of the config, so I decided to use the same system that was used and worked.

This comment has been minimized.

@eauchat

eauchat Feb 21, 2019

Author

there's no 'theme' property yet defined apparently

ah yes I guess if you've never defined the theme from the command line with the other PR in yunohost-admin it will not be defined I guess

This comment has been minimized.

@eauchat

eauchat Feb 21, 2019

Author

before logging in

I didn't test pre-logging, true. But yes it would be nice that the theme extends to the login page. Maybe we can figure out another way to pass the chosen theme setting, but this is the part of code that I understand less.

@alexAubin alexAubin referenced this pull request Feb 22, 2019

Open

Theming reloaded #122

3 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment