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.

Sign up for GitHub

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

Jump to bottom

Respect theme avatar-background-colors #26285

Closed
aceArt-GmbH opened this issue Sep 13, 2023 · 19 comments · Fixed by matrix-org/matrix-react-sdk#12240
Closed

Respect theme avatar-background-colors #26285

aceArt-GmbH opened this issue Sep 13, 2023 · 19 comments · Fixed by matrix-org/matrix-react-sdk#12240
Labels
A-Avatar A-Themes-Custom Custom theme variables or support A-Theming O-Occasional Affects or can be seen by some users regularly or most users rarely S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect X-Regression Z-Labs

Comments

@aceArt-GmbH
Copy link
Contributor

On element 1.11.42, the theme avatar-background-colors do not seem to be respected for avatar colors.

The CSS variables are set as --avatar-background-colors_0 but never read.

It would be nice to support theming again with the new design system
@aceArt-GmbH aceArt-GmbH mentioned this issue Sep 14, 2023
Merged
3 tasks
@aceArt-GmbH
Copy link
Contributor Author

I was told by @t3chguy that avatar-background-colors is kind of deprecated and the avatar colors should now inherit from the nickname color.
But setting username-colors in custom theme does not change the avatar color.
Compound does not seem to make use of --username-colors_0 or $username-variant1-color

@germain-gg
Copy link
Contributor

I'll be transfering this issue to element web as the problem is not with compound but rather an issue with the implementation inside Element Web.

@germain-gg germain-gg transferred this issue from element-hq/compound Oct 3, 2023
This was referenced Oct 9, 2023
Merged
Open
@robintown robintown added X-Regression A-Avatar T-Defect S-Minor Impairs non-critical functionality or suitable workarounds exist A-Theming A-Themes-Custom Custom theme variables or support O-Occasional Affects or can be seen by some users regularly or most users rarely labels Oct 13, 2023
@estellecomment
Copy link

The colors are now hardcoded in matrix-react-sdk, so they ignore the theme customisation :
https://github.com/matrix-org/matrix-react-sdk/blob/81c62e51c474581c3d3f0f1e72aa0d1eb26ac45a/src/Avatar.ts#L30

@aceArt-GmbH
Copy link
Contributor Author

@estellecomment hm, shouldn't the variables still be read:
https://github.com/matrix-org/matrix-react-sdk/blob/develop/src/Avatar.ts#L112-L114 ?

@t3chguy t3chguy mentioned this issue Nov 14, 2023
Closed
@PeterKleinTele2
Copy link

Not sure if this is the correct way to make my opinion heard, but... being able to control the colors of the imageless avatars is very important to my company. We are creating a suite with multiple collaboration tools which must share the same look-and feel throughout the suite. We had been able to reach a point with the old system where it looked good enough for our UX-department, now however this is broken and it has become a blocker for launching the suite. Please let us know if/when this can be rectified. Looking through the css it looks like a quick fix to add this support just by tweaking the CSS and allow for more variables inside config.json

@t3chguy
Copy link
Member

t3chguy commented Dec 14, 2023

The Avatars are drawn by https://github.com/element-hq/compound-web and Element Web can't override them as there's no variables other than the ones which represent the raw colour palette. It'd be up to the people working on that layer to unblock this.

@giomfo
Copy link
Member

giomfo commented Dec 15, 2023

Thx @t3chguy, would it be possible for you to create the issue in the compound-web repo and link it here?
I noticed that the original issue was created there, but it has been transferred here...

@t3chguy
Copy link
Member

t3chguy commented Dec 15, 2023

@giomfo I don't have enough context on it. Germain previously said its an implementation bug. I originally instructed the reporter to report it to Compound and then it was transferred. So clearly my context is incorrect.

@langleyd
Copy link
Member

@robintown , Could you provide any clarity from the compound side regarding @t3chguy 's comment above?

@robintown
Copy link
Member

Two ways of unblocking this that I see are:

  • Override Compound's .avatar[data-color="..."] rules in Element Web. This seems like a very brittle solution since the data-color attribute is likely intended to be an implementation detail of Compound Web, not a part of the public API. In fact, there are comments in the sorts code suggesting that the implementation might change here as soon as support for the CSS attr() function takes off.
  • Create overridable design tokens for the avatar colors, and modify the Compound implementation to consume them. I'd like to have a chat with the designers to make sure that this is sensible, and figure out what type of tokens would work best (semantic? component-level?), so this is something that will have to wait until the new year. This should make it trivial for Element Web to set custom colors.

@PeterKleinTele2
Copy link

Hello

This issue seems to have stuck here. @robintown you mentioned that you'd like to "have a chat with the designers". Do you know who we need to contact for this and is there any way I can help with pushing this forward?

I would be very grateful for any feedback

@robintown
Copy link
Member

Hi @PeterKleinTele2, thanks for the reminder, I admittedly lost track of this over the new year but will bring it up with the Compound team next week. I don't think there's anything that you can help with externally on this issue at the moment, but I'll try to get this unblocked and put the design tokens in place soon.

@PeterKleinTele2
Copy link

Hello @robintown , have you had any luck with the design team to see if this is fixable?

@robintown
Copy link
Member

@PeterKleinTele2 Hello, yes, after aligning our approach last week, the design team has now created the design tokens that I recommended to unblock this, and I will be creating the necessary theming mechanisms in Compound this week. Then there remains one last task in Element Web to wire up those theming mechanisms to its existing config.json custom themes system, which I expect we can do trivially.

Even though this issue is about a single component, we're currently seeing a need to establish a scalable approach to theming all of Compound, which is why we're trying to be more deliberate here. So, thank you for your patience! If not this week, I expect we can get those final pieces together before end of next week.

This was referenced Feb 1, 2024
Merged
Merged
@PeterKleinTele2
Copy link

Hello, it looks like you've come a long way towards fixing this in matrix-org/matrix-react-sdk#12209 but there is some check that did not complete. Looking forward to testing once this is resolved.

@robintown robintown mentioned this issue Feb 6, 2024
Merged
@robintown
Copy link
Member

Hi, this issue is not yet ready for testing, but it is continuing to move forward. I still expect the full Compound theming mechanism to be in place by the end of this week.

@robintown robintown mentioned this issue Feb 9, 2024
Merged
@robintown
Copy link
Member

This is resolved now, please see the updated theme documentation for links to the full list of customizable tokens, but the gist is that you can now override avatar and username colors like this:

{
  ...
  "setting_defaults": {
    "custom_themes": [
      {
        "name": "My Theme",
        "is_dark": false,
        "fonts": {
          ...
        },
        "colors": {
          ...
        },
        "compound": {
          "--cpd-color-text-decorative-1": "var(--cpd-color-red-1100)",
          "--cpd-color-text-decorative-2": "var(--cpd-color-orange-1100)",
          "--cpd-color-text-decorative-3": "var(--cpd-color-yellow-1100)",
          "--cpd-color-text-decorative-4": "var(--cpd-color-green-1100)",
          "--cpd-color-text-decorative-5": "var(--cpd-color-blue-1100)",
          "--cpd-color-text-decorative-6": "var(--cpd-color-purple-1100)",
          "--cpd-color-bg-decorative-1": "var(--cpd-color-alpha-red-400)",
          "--cpd-color-bg-decorative-2": "var(--cpd-color-alpha-orange-400)",
          "--cpd-color-bg-decorative-3": "var(--cpd-color-alpha-yellow-400)",
          "--cpd-color-bg-decorative-4": "var(--cpd-color-alpha-green-400)",
          "--cpd-color-bg-decorative-5": "var(--cpd-color-alpha-blue-400)",
          "--cpd-color-bg-decorative-6": "var(--cpd-color-alpha-purple-400)"
        }
      }
    ]
  }
}

@PeterKleinTele2
Copy link

PeterKleinTele2 commented Feb 13, 2024 via email

@robintown
Copy link
Member

The change will be available in tomorrow's Nightly build, yes, so that's likely the easiest way to test it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-Avatar A-Themes-Custom Custom theme variables or support A-Theming O-Occasional Affects or can be seen by some users regularly or most users rarely S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect X-Regression Z-Labs
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Enable custom themes to theme Compound robintown/matrix-react-sdk
8 participants
@germain-gg @estellecomment @t3chguy @langleyd @giomfo @aceArt-GmbH @robintown @PeterKleinTele2