custom theme refactor #7726
custom theme refactor #7726
Conversation
|
Sorry for the direct ping, but @SimonBrandner do you mind testing this PR out? I'd really like for this PR to be merged in so that custom themes can be easily worked upon. |
SimonBrandner
added
the
T-Task
There was a problem hiding this comment.
To be perfectly honest, I had a little trouble following what the PR is trying to do, perhaps some docs on what colours should go where would help.
As far as I can tell, the changes look mostly sane, though I am not exactly sure where we are heading.
I would appreciate it if someone else could have a look at this
| $primary-content: var(--primary-content, $primary-content); | ||
| $secondary-content: var(--secondary-content, $secondary-content); | ||
| $tertiary-content: var(--tertiary-content, $tertiary-content); |
There was a problem hiding this comment.
Why are these and a bunch of others no longer changeable?
There was a problem hiding this comment.
primary-content is redefined later on in the same file: https://github.com/matrix-org/matrix-react-sdk/pull/7726/files/21b8d90ca9cc4eb0e2b9332ccf70575d88fdcb19#diff-b489440c03bfaa8d465751827c8382ec7567b730e4362f9166d4c205942f3d49R68
There was a problem hiding this comment.
secondary-content would be redefined by secondary-fg-color by both the light and dark theme:
https://github.com/matrix-org/matrix-react-sdk/pull/7726/files/21b8d90ca9cc4eb0e2b9332ccf70575d88fdcb19#diff-ddc2c6eb3509b80549b30df3977708adfc119bfcea6e9253f88133791998666bL94
https://github.com/matrix-org/matrix-react-sdk/pull/7726/files/21b8d90ca9cc4eb0e2b9332ccf70575d88fdcb19#diff-90861a924b357562176a919e52e7efbe2f21df686d105dd439fcf244c9d51108L147
Thus no matter what the user defined for secondary-content it would not be respected in the theme. Since secondary-content = $secondary-fg-color is common for both light and dark themes it was moved into the newly created legacy_common file, which holds higher level css variables.
There was a problem hiding this comment.
tertiary-content has the same issue as secondary-content. Note a followup solution would be to clean up these variables are just duplicates are unnecessary but this PR focuses only on reordering the imports and keeping the same variables and behavior. Of course, this is just extra work if we want custom themes to be based off the current themes instead of the legacy themes, which is why I did not work on making the legacy themes as clean/streamlined as possible as I would rather do that for the newer themes. But fixing the import order is important for the current custom theming which is an issue I want to tackle and hence this PR.
There was a problem hiding this comment.
For other variables the reasons are similar, if you really want I go through and doc the reason for each variable. However, I would rather spend my time working on getting custom themes updated for the new themes instead of focusing on this codebase which will be deleted shortly.
| $primary-fg-color: #edf3ff; | ||
| $primary-bg-color: #181b21; | ||
| $muted-fg-color: #a1b2d1; // Commonly used in headings and relevant alt text |
There was a problem hiding this comment.
Why are we using the hex values here instead of variables?
There was a problem hiding this comment.
The hex values are used here since the hex value for these variables are used in _legacy-light.scss. (https://github.com/matrix-org/matrix-react-sdk/pull/7726/files/21b8d90ca9cc4eb0e2b9332ccf70575d88fdcb19#diff-90861a924b357562176a919e52e7efbe2f21df686d105dd439fcf244c9d51108R16)
_legacy-default contains hardcoded lower level variables (i.e. variables that other variables will be assigned to), _legacy-light/dark contain hardcoded lower level variables specified by that specific theme. Higher level variables are assigned to lower level variables via _legacy-common. _legacy-dark has a specific value of primary-fg-color and _legacy-light has a specific value of primary-fg-color that is unique to them. If a variable needs to be assigned to another variable than that same behavior should be replicated in the other theme, hence moving variable assignments into _legacy-common.
I apologize for that, but yes having docs is important. The goal here is to fix the import order so that variables properly propagate. In doing so, common hardcoded values are specified in I can create a follow up PR on this page updating it with my changes here: https://github.com/vector-im/element-web/blob/develop/docs/theming.md, but personally I would rather not, since the docs are not being updated anyway and I would rather not spend too much effort into something for which I will delete in a follow up PR (when I try tackling custom themes based on the new themes). If you really believe this is necessary I can write something temporary while custom themes are still based on the legacy themes.
Ultimately this fixing (the import order) will be needed for the current theme code when we have to incorporate custom theming into the new themes. Once that is accomplished, the legacy files will be removed.
pinging @aaronraimist as he is the maintainer of the custom themes repo Again I would like to mention this only effects custom themes as the legacy files are not publicly select able. |
|
I appreciate the ping as a heads up but I don't know enough about how themes are implemented to be able to provide feedback. |
|
@SimonBrandner what'll be the status of this PR? If there is more to discuss please free to ask me questions. Is there anyone else that needs to be pinged on this PR before it is merged? |
|
it needs review from the core team - Simon just happens to be a very involved community member. We'll get to it when we can, but have to consider quite a few factors before accepting such a large change. For example, we have themes that we support internally and need to determine if/how those would break with this change. |
Got it. |
I'd like to give this another look but didn't have the time yet. Also what T said |
acxz
force-pushed
the
custom-theme-update
branch
2 times, most recently
from
ac3ee48 to
7109be7
Compare
to make sure scss variables are being properly loaded and propagated the import order has been corrected. it is now changed to [common hardcoded values (_legacy-default) -> theme specific hardcoded values (_legacy-dark/light) -> custom variable values (_legacy-custom) -> common dependant values (_legacy-common)]. in order to facilitate this import order values that were hardcoded in _legacy-dark but not in _legacy-light are now hardcoded in _legacy_light and vice versa. variables were changed to be the exact value of the hex so no change in colors is in this commit. it is important to note that this code does slightly change the custom theming api. in particular it removes exposed variables in the api that were being rewritten (i.e. bugged variables) users of the api should not see anything change in theming only that some variables (that had no effect anyway) cannot be set".
acxz
force-pushed
the
custom-theme-update
branch
from
7109be7 to
316a9de
Compare
|
Hello all, I was wondering if anyone has gotten more time to review this change or at least the ideology/reasoning behind it. Would appreciate the feedback. |
Have you been able to get around to this? I had assumed that only custom themes were based on the legacy theming backend. However, with the recent merge conflicts, it seems that there is active work being done on the legacy themes. |
turt2live
added
the
X-Blocked
|
Unfortunately I think a change like this will require driving from an internal team of ours, as the maintenance burden and review cycle a community PR provides will take far too long or is easily forgotten (as it happened in this case). With the internal team driving it, we can better determine what the requirements of such an architectural change would be and manage the timeline a bit more closely. I do very much appreciate the effort that went into this, however it's just not something we can take the risk on at the moment. If the internal team(s) are looking for a place to start from, this would be it. Thank you for the contribution in any case! |
|
I understand, this is a large enough refactor where having an internal member is quite necessary. At first I thought that the custom theming engine is the only reason for the themes to exist, so I had assumed changes to the custom theming would be no risk to other users allowing for the easing merge, but based on above comments it seems like the legacy themes are being used internally at matrix.org Is there a timeline for removing the legacy themes? Another question I have, would you be able willing to accept a PR that changes the custom theming from the legacy backend to the new backend or adds a new custom theming along with the old custom theming? Ultimately my goal is to upgrade the custom theming on element and this PR was just the first step in fixing current custom theming issues before porting to the new theme backend. Any thoughts on this would be much appreciated. |
|
We're worried about the custom theme engine specifically, as many of our internally-supported custom themes rely on the behaviour offered by the legacy theme. We don't use the legacy themes outside of custom themes. So, unfortunately, I think even touching the custom themes would require a team member to look at it. |
|
Got it, so basically I shouldn't touch the current custom theming engine in a large manner. With the experience I gained creating this PR, I can however update the docs/theming.md file that @SimonBrandner originally wanted me to work on. One last thing, while I can't touch the current custom theming, would you be open to a PR where I create say a new_custom theme based on the current themes? These would not interfere at will the current custom themes/legacy themes. |
|
The theming engine should honestly only be worked on by the internal team, sorry. |
|
Got it thank you for the clarification! |
Slowly chips away at element-hq/element-web#13082, maybe want to close it? as element-hq/element-web#17348 encompasses that issue as well
maybe want to close the associated PR, #4373 as that is linked to the first issue mentioned and outdated
Overall this is a refactor of the legacy theme system. However this only effects custom theming as the legacy themes are not publicly accessible and the custom theming is still based on the outdated legacy theme. With this refactor the important thing to note is the fix of the import order. Due to how the scss files were being imported, some user defined variables were not being properly propagated.
Example after this PR, element-hq/element-web#20925 can be easily fixed by adding
$icon-button-color: $quaternary-contentto the newly created_legacy_common.scssfile. This PR also renders "hacky" solutions like manually adding every color variable a user wants the theme to the_custom.scss(now the_legacy_custom.scss) file like #7725 and fe7f453 unnecessary. (Although it is totally fine if we want to expose more color variables to the users, but maintaining a_legacy_common.scssfile with how high-level elements change with respect to low level specified colors likeprimary/secondary contentis a boon. Common hardcoded values are of course imported first and specified with_legacy_default.scss. With the_legacy_dark/light.scssthemes building/changing variables on top of them and_legacy_custom.scssfurther building on top`.After this PR is merged, the next step for custom theming would be to perform a similar refactor for the current themes and add custom theming based off the new themes so that the legacy themes can be deleted.
Another step can be to continue refactoring (and exposing more variables with) the custom theming using the legacy backed. This would involve grouping similar colors together and moving more variables to
_legacy_common.scssso that more elements can be changed with less color definitions. With how the code is now structure this would mean just a single lower level color variable (i.e.tertiary-content) in_legacy_dark/light/custom.scsswould change many higher level variables in_legacy_common.scssHowever, this work would ultimately be mute since the end goal is to have custom theming based on the new themes. And I personally would rather spend time performing such a refactor for the current dark/light/light-high-contrast themes.
See below for commit message.
to make sure scss variables are being properly loaded and propagated the import order has been corrected. it is now changed to [common hardcoded values (_legacy-default) -> theme specific hardcoded values (_legacy-dark/light) -> custom variable values (_legacy-custom) -> common dependant values (_legacy-common)]. in order to facilitate this import order values that were hardcoded in _legacy-dark but not in _legacy-light are not hardcoded in _legacy_light and vice versa. variables were changed to be the exact value of the hex so no change in colors is in this commit. it is important to note that this code does slightly change the custom theming api. in particular it removes exposed variables in the api that were being rewritten (i.e. bugged variables) users of the api should not see anything change in theming only that some variables (that had not effect anyway) cannot be set".
removed color variables from custom theming api:
primary-contentsecondary-contenttertiary-contentquinary-contentbackgroundrelevant people:
@aaronraimist @SimonBrandner @fooness
p.s. sorry for the wall of text
This change is marked as an internal change (Task), so will not be included in the changelog.
Preview: https://pr7726--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.