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.

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

(PoC/WIP)Theme selection #69

Merged
merged 9 commits into from Nov 22, 2018
Merged

(PoC/WIP)Theme selection #69

merged 9 commits into from Nov 22, 2018

Conversation

xse
Copy link
Contributor

@xse xse commented Nov 20, 2018

  • Live demo : http://krkrkr.org

  • Forum might not work atm (did not setup a phpbb clone yet)

So when a visitor comes if he doesn't have an already selected theme, this place a default object containing the default theme configuration in his browser storage.
He can switch themes using an html select thingy on the index, if he does the new theme configuration is applied & stored.
So far there is only one theme and it's just tweaks for the default one, so default is always on and the Halloween one gets enabled/disabled as needed.

To add a new theme you need to :

  1. Include it here as usual on the top of default.html & post.html
  2. Add the option inside the select thingy in index.php
  3. Add it in themes.js inside the user_themes_default object :
const user_themes_default = {
  0: true, // css.css will always be true as long as the design doesn't change.
  1: false // css-halloween.css, if true the Halloween theme is activated.
  2: true // possible new theme, activated on top of the default one.
};
  • TODO :

  1. I don't like how adding a new theme requires 3 separate actions, maybe use themes.json that once parsed does the other actions ( bloated ?... ).
  2. Optimizations/Cleanup comments and such.
  3. Checking the forum part, i did not look into that yet maybe that will work directly maybe not.
  4. The select html tag needs css beautification i'm bad at css.

Anyway, any thought ?

@def-
Copy link
Member

def- commented Nov 20, 2018

Great!

About 1: How about just storing which additional css file should be loaded as a string like "css-halloween.css"? But then you'd have to be careful not to allow including a css from an untrusted domain.
About 3.: Don't worry about the forum.

@edg-l
Copy link
Member

edg-l commented Nov 20, 2018

Maybe you could turn the select into a simple switch button that clearly shows which is on/off.

https://www.w3schools.com/howto/howto_css_switch.asp

@def-
Copy link
Member

def- commented Nov 21, 2018

Ready to be merged? Could it share the forum setting even though the forum is in forum.ddnet.tw instead of ddnet.tw?

@xse
Copy link
Contributor Author

xse commented Nov 21, 2018

nope some cleanup to do i left some console.log calls and i think i need to handle the case when default_theme is set to something, will be soon tho

EDIT : for the forum i don't know the link is as "/thefile.css" so if that file exists at the / of forum.ddnet.tw it should work

@xse
Copy link
Contributor Author

xse commented Nov 21, 2018

Indeed i did not thought about the forum, it's on a different subdomain i'll have to rewrite the storage part for it to to use a cookie.

code is way simpler but with limitations :

  1. Only one theme available ( it's a checkbox )
  2. Default theme is only useful for users that don't already have a theme stored. ( e.g if user prefers no theme he'll keep using no theme even if there is a new default_theme )
  3. Users that don't have js activated will always use no theme (they never download the css) whereas as it is now they do.

Are any of those problematic/needed ?

To add a new theme just modify the available_theme variable.

var available_theme = "/css-halloween.css";
const default_theme = "";

Alright i'll start on the cookie storage part by tomorrow have a good day !

@def-
Copy link
Member

def- commented Nov 21, 2018

Thanks a lot, sounds all good to me :)

@xse
Copy link
Contributor Author

xse commented Nov 21, 2018

http://foo.krkrkr.org/ - http://bar.krkrkr.org/

As long as the script is called in the page and somefile.css is at / of that domain/subdomain it should work, did not do extensive testing tho

@def-
Copy link
Member

def- commented Nov 21, 2018

The switch stays on dark when you switch one to bright but still have the other tab open and press f5 there, even though the color changes.

@xse
Copy link
Contributor Author

xse commented Nov 21, 2018

Should be fixed 👍

@def-
Copy link
Member

def- commented Nov 22, 2018

I'd like to merge it and see how it works in production. Anything you still want to clean up before that?

@xse
Copy link
Contributor Author

xse commented Nov 22, 2018

nope i mean can change default_theme if you want the default to be halloween style otherwise it's fine.
I'd still advise backup in case that breaks something as i can't test extensively on a "real" environment like on the pages i don't have in my copy, the forum and so on.

edit : demo not available atm server in maintenance for a defective memory module.

@xse
Copy link
Contributor Author

xse commented Nov 22, 2018

@def- for example right now on ddnet.tw, https://ddnet.tw/ranks/ gets the halloween css but if i search my nickname https://ddnet.tw/players/-9835-/ i don't get that css.

The /ranks directory does not exist in that repo, & the thing is /players exists and is apparently based on the default layout, where the css is included, so there's an issue here. Must have been generated before that halloween css were included or something like that, i don't know how that's organized in production

@def- def- merged commit e92a4fd into ddnet:master Nov 22, 2018
@def-
Copy link
Member

def- commented Nov 22, 2018

Huge thanks! I'll fix it up for forum and scripts now :)

@def-
Copy link
Member

def- commented Nov 22, 2018

Hm, doesn't seem to work with the forum. And it's a bit annoying that it first loads in the bright color and then switches to dark.

@xse
Copy link
Contributor Author

xse commented Nov 22, 2018

Doesn't work on the forums because themes.js isn't at the forum /

Yeah indeed the flickering can get annoying i'm gonna try to see if i can fix it as soon as i get a bit of free time

@def-
Copy link
Member

def- commented Nov 22, 2018

Doesn't work on the forums because themes.js isn't at the forum /

Fixed, still not working.

@def-
Copy link
Member

def- commented Nov 22, 2018

Actually the forum had its own halloween style, I guess it'll be hard to make that switch too. So no worries.

@xse
Copy link
Contributor Author

xse commented Nov 22, 2018

I mean there is code for the forum on the halloween file.
Is css-halloween.css avaiable at the / of that forum ?

@def-
Copy link
Member

def- commented Nov 22, 2018

Ah nope, that's also at //ddnet.tw/css-halloween.css. Should I symlink it or what's the easiest fix?

@xse
Copy link
Contributor Author

xse commented Nov 22, 2018

As you wish either symlink it or otherwise themes.js could checks for the url and add "//ddnet.tw//css-halloween.css" if it is on the forums, same results i guess

@def-
Copy link
Member

def- commented Nov 22, 2018

Great, works! Should I write a news article for this or should we wait to see if you can fix the flash first?

@xse
Copy link
Contributor Author

xse commented Nov 22, 2018

yeah i'll try to fix that first, no needs for an article about that 👍

@xse
Copy link
Contributor Author

xse commented Nov 22, 2018

Ok solution to fix flickering basically involves adding the halloween style on post & default and enabling/disabling in using js instead of adding it only if needed

edit: demo should work without flickering atm
edit : see #70

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants