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

[shelter-ui] Button text uses the wrong variable #39

Open
mangkoran opened this issue Nov 27, 2024 · 7 comments
Open

[shelter-ui] Button text uses the wrong variable #39

mangkoran opened this issue Nov 27, 2024 · 7 comments

Comments

@mangkoran
Copy link

mangkoran commented Nov 27, 2024

I found that Button has low contrast if user use dark theme.

Example with catppuccin-mocha. Both client are in dark mode.

Shelter (in Dorion client)
image

Compared to Vencord (in official Discord client). This is the same as Discord's default button style
image

I expect Button style should be similar to Discord's default button style in dark theme. But correct me if this is intended.

@yellowsink
Copy link
Member

This theme overrides classes specific to the Discord button, instead of overriding the relevant css variables, which would work fine in shelter.

This is not intentional behaviour as such, but we cannot realistically support every theme due to cases like this where they just do things in different ways.

This is a natural consequence of us remaking all of Discord's UI components instead of using them verbatim, which is a very necessary and non-negotiable part of our stability model,
so unfortunately I can't offer to fix this case.

image

In fact, this theme appears to even be inconsistent in its presentation with Discord's buttons!

image

Sorry.

@yellowsink yellowsink closed this as not planned Won't fix, can't repro, duplicate, stale Nov 27, 2024
@mangkoran
Copy link
Author

mangkoran commented Nov 28, 2024

Thank you for your reply. You are correct that it seems it's due to catppuccin theme isn't using --button-filled-brand-text variable as button text color. However, it seems shelter-ui is also not using --button-filled-brand-text as button text color. Instead shelter-ui button use --interactive-active.

BRAND: ["var(--brand-500)", "var(--interactive-active)", "var(--brand-560)"],
RED: ["var(--button-danger-background)", "var(--interactive-active)", "var(--button-danger-background-hover)"],
GREEN: ["var(--button-positive-background)", "var(--interactive-active)", "var(--button-positive-background-hover)"],

image

And it seems --interactive-active is also used in other component such as text in selected friend in friend list. This is how it looks if I set --interactive-active to #000000.

image

Compared to the theme default
image

May I know which one is the correct variable to use for button text color? Is it --button-filled-brand-text or --interactive-active? I could help to make changes in catppuccin repo regarding the correct variable usage.

@yellowsink
Copy link
Member

I do believe shelter UI is wrong about which variable to use here, it should be button-filled-xxxx-text, and I do plan to change this, but this theme would still sadly be broken

@mangkoran
Copy link
Author

but this theme would still sadly be broken

It's okay no worries I could help to discuss this in catppuccin repo. Nice to hear that it's acknowledged at least. Maybe we could reopen the ticket to track the fix for shelter-ui? 👀

@yellowsink
Copy link
Member

that is actually a sensible idea yeah sure

@yellowsink yellowsink reopened this Nov 28, 2024
@yellowsink yellowsink changed the title [shelter-ui] Button has low contrast in dark theme [shelter-ui] Button text uses the wrong variable Nov 28, 2024
@mangkoran
Copy link
Author

Speaking of themes, is there any reference theme implementation that we could use as base/reference for making themes? So far I found BetterDiscord variables but let me know if there's better/more preferable reference.

@yellowsink
Copy link
Member

I'm not aware of any better resource, or any resources at all really for theming other than just seeing what others do and mimicking that.

quite honestly my method back when I did theming was just to find the thing I wanted to change and see what variable it used, then see if overriding that had the desired effect without breaking everything else.

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

No branches or pull requests

2 participants