-
-
Notifications
You must be signed in to change notification settings - Fork 827
Fix color definitions, only use variables to allow proper custom theme support … #4373
Fix color definitions, only use variables to allow proper custom theme support … #4373
Conversation
This is a little difficult for me to review on behalf of @matrix-org/design as it looks to address tech debt, and concern how we architect our CSS. @fooness Should this PR produce any visual changes? @jryans do you have any input on how the PR may impact theming opportunities in future? How should we be reviewing this? Ad hoc build? |
@nadonomy No visual changes in default theme is perfecftly fine. When customizing the theme via
In the long run, it probably would be good to make some changes to the structure of Maybe something like: element-hq/element-web#12314 Also, I wish the following issue would be fixed rather soon. When this happend, I feel like I could color-change every element I wanted, and I hope that others would confirm … element-hq/element-web#12630 |
Added |
Added |
Added |
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.
Overall, this looks reasonable to me! It cleans up the custom variables and applies them to some spots that may have been missed or added later. There should be no impact to the default themes.
Mostly I'd like to know more about how existing custom themes might be affected first.
$accent-color-alt: var(--primary-color); | ||
$input-focused-border-color: var(--primary-color); | ||
// --username-color | ||
$username-variant1-color: var(--username-color); |
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.
Here and in a few other places, you've added new variables to be set by the custom theme. Was it the effect for older custom themes that don't have a value username-color
set?
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 effect for older custom themes which don’t set this css variable is … none, I’m pretty sure? If the default theme is used, the custom variables are ignored all together. If a custom theme is used, then only the defined custom variables (in config.json
) are changed, all non-defined variables just fall back to their default values. Right?
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.
If a custom theme is used, then only the defined custom variables (in
config.json
) are changed, all non-defined variables just fall back to their default values. Right?
I don't think this is true. SASS definitiions like $username-variant1-color
are compile-time, and they can't fall back on previous definitions of the same variable depending on the runtime value of a CSS variable.
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 wonder if $username-variant1-color: var(--username-color, $username-variant1-color);
would work though, using the default value argument in var
to refer to the previous value of the SASS variable.
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 wonder if
$username-variant1-color: var(--username-color, $username-variant1-color);
would work though, using the default value argument invar
to refer to the previous value of the SASS variable.
Having carefully read through the specs of CSS var()
, I think you’re pretty right with this proposal.
Do you have a quick way to absolutely validate this?
Overall this shouldn't affect the future of theming much. It adds one or two new colours for custom theme users and default themes are unaffected. Maybe the screens above are good enough for you, or else please request an ad-hoc build from @fooness. |
…buttons (now accent-color, was falsely timeline-text-color before)
…buttons (now button-color, was falsely timeline-text-color before)
…ing $topleftmenu-color
This last fix was quite challenging in the end, but now it’s working properly (without breaking or modifying anything unintended, for sure). Unfortunately the key was slighly touching a second file, I changed the following …
… to the solution below:
Sorry for the hustle in the last minute. My last change does not negate anything written by @jryans above. |
@fooness are these the CSS variables that are added in this PR?:
Are there any others I missed in the PR? I like the addition of font variables! I'm not 100% sold though that having one custom variable for all user name categories is better than not being able to theme them at all and relying on the default, as the point of colouring display names is to be able to visually distinguish (similar) display names. Wrt to using using the existing CSS variables in more places, we probably need to verify to some extent that this doesn't break other themes too much, as they might rely on the built-in values. The font-family part (if we get fallback to the built-in font working) should be straightforward to get in though. If you would like to get this part in quickly, feel free to PR it separately. |
// --roomlist-highlights-color | ||
$roomtile-selected-bg-color: var(--roomlist-highlights-color); | ||
// --font-family | ||
$font-family: var(--font-family); |
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 a default here for older custom themes. Does this make it fall back to the default Nunito if --font-family
is empty?:
$font-family: var(--font-family, $font-family);
@bwindels That’s true.
No, there’s no others.
Great!
They don’t have to be modified if one is okay with the default. But also it would be possible to have eight variables for What do you prefer?
I don’t think that might happen, as the very most of the other variables where not part of |
Hope I did that correctly. Accidently clicked |
These files provided values for the SASS variables that are now being set with one of the existing CSS variable in this PR. The CSS variable might not be the same value as what it had before. So for example, some themes might look a bit off now that |
Oh okay, that’s what you mean. Please let me refer to the https://github.com/aaronraimist/riot-web-themes repo and provide some screenshots.
Having made the styling (and grouping and wording) of variables more consistent and complete, I think it’s possible that at first glance some custom themes might look slightly different to before. But with adding the new variables it should be possible to get a result similar to the current custom theme status but more complete support throughout the team. I cannot guarantee for this, so, I guess, every other contributor cannot guarantee that nothing will change in the theming for like ever. Maybe someone with custom themes could check this for themselves and validate? Overall, there’s still lots of things that need to be done for absolute theming support. This pull request is just the beginning and basically shows what was possible with only changes to the EDIT: And … please don’t get me wrong. I’m more than happy about contributions, feedback, and overall progress in the theming possibilities of riot-web. But, I fear, at some point there’s more to change than just adding some variables here and there to achieve the best result and make theming as easy as possible. But that’s something that’s probably already on the roadmap for the devs and I fear that also is more complicated than I’m capable of (and also needs some governance and consensus among the devs and the community). |
// --roomlist-highlights-color | ||
$roomtile-selected-bg-color: var(--roomlist-highlights-color); | ||
// --font-family | ||
$font-family: var(--font-family, $font-family); |
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 will also need a check to not calculate different alpha values for font variables in theme.js
I'm working on something myself that needs custom theming support, which some changes that may overlap with this PR. Will comment here when I know more. |
See #4503 for my changes. I've implemented the styling of usernames slightly different there. See element-hq/element-web#13406 for doc updates of how it is supposed to work. |
Sounds and looks great. Thank you! What should I do now? Remove the usernames variables from my pull request? |
Yeah, that would probably be easiest. Thank you. |
…-color-definitions
Should be done. |
Just wary this is still outstanding in the @matrix-org/design queue. @bwindels pls could you tag me into a comment when this has passed code review and I can review from there? |
…atch reaction pill tooltip)
Unify reaction-pill-tooltip and text-input-markdown-tooltip on :hover … that is, in detail, one change and one add: Make
Add
— Also: added updates from @bwindels … that is: b187066, 2f68f60, and 8b8b525 (with |
@bwindels As this PR seems to be pretty abandoned by now, may I kindly ask you to add the possibilities of custom font-families (sans-serif and monospace) and custom button colors (see below, still the same problem as ever) — fixed in this PR — to your next PR?
Probably forgot some elements … all of those — if I remember correctly — would have been fixed by this PR but are still not-styleable in Riot-Web 1.6.0–1.6.4 … which is pretty sad for everyone not using the default theme (and I’m pretty sure that’s many people). |
is being tracked at element-hq/element-web#9403 |
I'm working on font support for custom themes in #4814 fwiw |
So, apologies for the long delay. After having another look at this, there are at least the following things that prevent this from getting merged:
In general, when making changes in these parts that are a bit more fragile like custom theming, I would encourage to explain in great detail why you're making the changes you made, and what checks you have done to make sure it does not break anything. Smaller PRs that don't do several things at once also have a higher changes of getting through quickly. Thanks for bearing with us. |
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.
See comment above
@turt2live should this be closed as well in lieu of #7726 (comment) ? |
I think so, though for this one I think it's age alone which makes it impossible for us to reasonably merge this. Thanks for spotting! |
See previous discussion in element-hq/element-web#13082 …
This pull requests fixes some scss variable <-> css variable color definitions to improve custom theme support via
config.json
… and also adds the possibility to define customfont-family
andfont-family-monospace
viaconfig.json
… plus the variable list was ordered/sorted by alphabet for each section, as this was most of the times the case, but sometimes not.—
Signed-off-by: fooness
Example usage: