-
-
Notifications
You must be signed in to change notification settings - Fork 172
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
Theming efforts #911
Theming efforts #911
Conversation
1e95ba7
to
35be9a4
Compare
at this commit rate we should squash merge them in the end.. |
bc67f70
to
75d83d0
Compare
Edit: pls. refer to the new todo section in src/renderer/ThemeSystem.md |
I found a bunch more places where there are missing vars, namely: the text input field and names in the contact list |
scss/_variables.scss
Outdated
@@ -38,36 +38,151 @@ $roboto-light: Roboto-Light, $emojifonts, 'Helvetica Neue', Arial, Helvetica, No | |||
// New colors | |||
|
|||
html { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These here act as fallback, but I think about removing them to save a few lines and reduce duplication..
``` | ||
|
||
|
||
### TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the todo is here now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Jikstra have you read this file?
src/renderer/ThemeBackend.js
Outdated
console.log('dark') | ||
return color.lighten(factor).rgb().string() | ||
} else if (color.isLight()) { | ||
console.log('light') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
console.log, maybe a log.debug if it's not too spammy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the plan is to remove it after we fixed a problem with this function
scss/_variables.scss
Outdated
$color-message-author: var(--clr-message-author); | ||
$color-message-attachment-container-bg: var(clr-message-attachment-container-bg); | ||
$color-message-attachment-filename-incomming: var(--clr-message-attachment-filename-incomming); | ||
$color-message-attachment-filesize-incomming: var(--clr-message-attachment-filesize-incomming); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
incoming only needs one m
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you want to have different color shades for filename and filesize... but for this one we might get away with setting opacity for this effect, then we only need one var and if message-text-collor == filename-color == filesize-color we don't need any extra var for it.
scss/_variables.scss
Outdated
--clr-message-generic-attachment-filesize: #070c14; | ||
--clr-message-attachment-container-bg: #ffffff; | ||
--clr-message-attachment-filename-incomming: #ffffff; | ||
--clr-message-attachment-filesize-incomming: #ffffff; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me it looks like --clr variables are the real base and they just get pulled in into the --color variables which allow a more specific adjustment. If this is the case, i would prefer to have only one --clr variable for every value and give it a proper name. For example there should be no need to have message-text and message-text-underline variable, that's way to verbose for base colors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$colors you mean? we used this to resolve and group them centraly, we might resolve them again (put the var(--clr-component-name)
directly in the components).
For example there should be no need to have message-text and message-text-underline variable
I think one might want to change the link color independently from the message text color (telegram has a different color for links)
), | ||
scrollBarThumbHover: undefinedGuard( | ||
theme.scrollbarTransparency, c => Color('black').alpha(c + 0.14).rgb().string() | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how much work this adds that if we want to define a new variable, we need to hardcode it at several places. Also it seems to me that we can't override those variables here from a theme?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to hardcode it once for styled components and 3-4 times for scss
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also for scss: we could just transfer the ("highlevel") theme vars and resolve them in scss, but some of them need additional transformation that must be done in js, because at runtime we don't have the fancy scss functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Simon-Laux what kind of additional transformation do they need?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The actual theme gets generated here from the "highlevel" theme the user specified, after that when the "raw" object is set, its values replace the the generated values, so you still can have whole controll of it.
--clr-bp3-button-bg: ${theme.bp3ButtonBg} | ||
--clr-bp3-dialog-header-bg: ${theme.bp3DialogHeaderBg} | ||
--clr-broken-media-bg: ${theme.brokenMediaText}; | ||
--clr-broken-media-bg-text: ${theme.brokenMediaBg}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even more hardcodes with different naming conventions and names, that's confusing. Can we streamline this? For example start using camelCase everywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is setting the css variables and as far as i know the naming for those is --[name in lowercase + '-']
https://developer.mozilla.org/en-US/docs/Web/CSS/Using_CSS_custom_properties
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would propose following ideas:
- use snake case name everywhere
- use snake case for css custom properties, automatically (with help of a method) translate it into a camelCase name
- use camelCase everywhere as it seems to work: https://codepen.io/anon/pen/xvXPWN?&editable=true (what i prefer, less problems with standard)
Another thing that i really would like to have is, having an automated way to inject the theming variables into the various parts, so that you don't need to hardcode the names everywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you only need to "hardcode"/define it in one place and thats ThemeBackend.js
, I don't see it as big deal. Also then you have the advantage that you basicaly have a directory of every themeable var which you can use as reference for creating a theme.
If you have an scss var you define it twice sometime additionally in _variables.scss
we could probably resolve the ones in _variables.scss
so we only have the ones left in ThemeBackend.js
.
You're right we could generate the var overwrite to save even more space. I'll try some things.
We merge this now, but keep in mind that there are still somethings left to do for good theming. |
components anyway
in preparation to add them to theming
to work better with different backgrounds
Co-authored-by: Simon Laux <[email protected]>
5855bcf
to
c99aa8e
Compare
``` | ||
Quirks: | ||
- You're theme gets only updated and loaded on change of the specified theme file | ||
- Doesn't work with `npm run dev` because this argument gets lost |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should fix this to make npm run dev allow arguments. We probably need to use a dev.js script to make this work.
The goal of this pr/sprint is to
1. Centralize all component color vars (no general non-telling var anymore)
2. Create an abstraction layer with vars that have meaningfull names and convert from them to the component colors.
3. Create some sample themes to test if we did it right
If Move LoginScreen in own file, fix importing backups #863 is getting merged we need to change the deltaFocusBlue variable name