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

Add Darkmode (#766) #778

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Add Darkmode (#766) #778

wants to merge 9 commits into from

Conversation

zeejot
Copy link

@zeejot zeejot commented Oct 13, 2019

  • improve settings/options
  • add darktheme and switch in options
  • update css file

* improve settings/options
* add darktheme and switch in options
* update css file
@andig
Copy link
Contributor

andig commented Oct 13, 2019

This PR changes all indents which makes it very hard to follow what the actual changes are. Would you want to clean up and also add a screenshot? Is it possible to detect dark mode from browser side on OSX instead of forcing it by user option?

@zeejot
Copy link
Author

zeejot commented Oct 13, 2019

Screenshot Dark/Light Theme:

vz

Indents are removed, where possible.

@zeejot
Copy link
Author

zeejot commented Oct 13, 2019

Automatic dark mode detection like in https://github.com/manuelottlik/ct-css-darkmode seems not to work. :-(

@andig
Copy link
Contributor

andig commented Oct 14, 2019

Did you see https://stackoverflow.com/questions/50840168/how-to-detect-if-the-os-is-in-dark-mode-in-browsers#50843966. It would be nice to only have a manual setting when not running on OSX?

@zeejot
Copy link
Author

zeejot commented Oct 14, 2019

Test Site for dark theme browser support:
https://codepen.io/kleinfreund/pen/NmpKZM

It's not working for Edge browser, but for Chrome etc.

@andig
Copy link
Contributor

andig commented Oct 14, 2019

CSS variables only work as of Edge 15, released 2017. I guess that's acceptable?

@zeejot
Copy link
Author

zeejot commented Oct 14, 2019

Jop and there is also the manual override.

@frankrichter
Copy link
Contributor

What happened to the "eye" icon (toggles channel visibility)?

@zeejot zeejot closed this Oct 16, 2019
@zeejot zeejot deleted the darkTheme branch October 16, 2019 20:59
@zeejot zeejot restored the darkTheme branch October 16, 2019 21:01
@zeejot zeejot reopened this Oct 16, 2019
…into darkTheme

# Conflicts:
#	htdocs/css/style.css
@zeejot
Copy link
Author

zeejot commented Oct 16, 2019

Good evening,

found some bugs with links and margins that are fixed now.

Do you think it makese sense to update the icons too? Maybe to fontawesome or something else?

@andig
Copy link
Contributor

andig commented Oct 17, 2019

Do you think it makese sense to update the icons too? Maybe to fontawesome or something else?

I would really love to replace our icons with an SVG icon font to get a consistent look and feel. Sofar I have not been able to find one that would especially delivery good symbols for all the different channel types.

Long story short: a PR would be great, but should be separate from this one.

@andig
Copy link
Contributor

andig commented Oct 17, 2019

Personally, I don't like the new striped table look (especially inside dialog boxes). I didn't have time yet to check the look on Mac but isn't the redesigned slider another, separate PR?

@zeejot
Copy link
Author

zeejot commented Oct 17, 2019

I totally agree with you - the striped table look in the dialog boxes was ugly. I fixed that with last change.
Yes the Icons are a seperate PR. Maybe I will take a look at this topic after that one :-)

Do you mean the toggle switch with redesigned slider?

@zeejot
Copy link
Author

zeejot commented Jan 11, 2020

Hi Andig,
was fehlt denn noch um es in Standard mit aufzunehmen. Kann ich noch was beisteuern. Sind es nur die Checkboxen unter den Settings?
Viele Grüße

@andig
Copy link
Contributor

andig commented Jan 11, 2020

I tend to not merge this PR. A goal of UI refactoring was to get rid of the settings in an effort to make the UI cleaner. This PR introduces another setting. If it helps we could merge the CSS variables instead to make it easier for a fork to implement the dark theme.

@Feilner
Copy link
Contributor

Feilner commented Jan 12, 2020

Most OS supports nowadays a dark mode. Like Android, iOS, Windows 10 also browsers like Chrome supports the dark mode. Some also time triggered. So for a good user experience also web pages should support the dark mode. Even if the dark mode auto detection does not yet work in all OS/Browser combinations this will for sure improve in future.

@andig What did you mean with "This PR introduces another setting"? The toggle switch to manually enable/disable the dark mode? Or the line in options.js?

I don't think they are a reason not to merge PR. Especially since they have been present since the beginning of PR. And zeejot invested a lot of work.

@andig
Copy link
Contributor

andig commented Jan 12, 2020

What did you mean with "This PR introduces another setting"? The toggle switch to manually enable/disable the dark mode? Or the line in options.js?

The switch

I don't think they are a reason not to merge PR.

This PR totally changes the appearance of the UI, even in normal mode, while claiming to "add" a dark mode. As such it is maximally invasise and won't get merged, at any rate as-is.

@isarrider
Copy link

I also hacked sth together...
style.txt
Not for merging - I would have been happy to find sth immead ;)

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

Successfully merging this pull request may close these issues.

5 participants