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

Add custom -bd-accent-color css value #1663

Merged
merged 5 commits into from
Aug 30, 2023

Conversation

TheCommieAxolotl
Copy link
Collaborator

@TheCommieAxolotl TheCommieAxolotl commented Aug 29, 2023

Closes #970.

Adds a special --os-accent-color variable that themes/customcss can use.

e.g.

:root {
	--theme-accent: var(--os-accent-color);
}

@TheCommieAxolotl
Copy link
Collaborator Author

Video Preview:

accent.mov

@rauenzi
Copy link
Member

rauenzi commented Aug 29, 2023

I understand why you prefixed it with -bd but I think the name is a bit misleading. Maybe something like --os-accent-color. I also think it would be better to just add a snippet like this

:root {
   --os-accent-color: <result of ipc>;
}

rather than trying to manually process and replace every css added through the DOM manager as that adds additional overhead (and makes it unexpectedly asynchronous) and does not catch all cases.

@TheCommieAxolotl
Copy link
Collaborator Author

I've updated it to do that, the only issue is that there might be a noticeable delay before the colour "loads" as it's executing after the rest of the startup process however that would only be a problem if a theme used this value extensively.

@rauenzi
Copy link
Member

rauenzi commented Aug 29, 2023

Yeah that's a fair point, and I don't have a problem with it being moved to before themes in the core order and done via then so we don't necessarily have to wait.

IPC.getSystemAccentColor().then(value => DOMManager.injectStyle("bd-os-values", `:root {--os-accent-color: ${value}));

Or is there something else that prevents us from doing it that early?

I also thought about whether this should be a user setting, but I think making it a setting would end up with too much inconsistency.

My only other thought is, what do you think about returning "" as default value instead of #000000 so themes can make use of variable fallback (var(--os-accent-color, #f0f0ff))? Or maybe #000000 is an electron defined default thing.

@TheCommieAxolotl
Copy link
Collaborator Author

I also thought about whether this should be a user setting, but I think making it a setting would end up with too much inconsistency.

That could be done, but yes I do agree it would add a level of complexity.

@rauenzi
Copy link
Member

rauenzi commented Aug 29, 2023

Yeah if you think it's not worth it then let's not bother. It LGTM otherwise.

@rauenzi
Copy link
Member

rauenzi commented Aug 29, 2023

Actually I think there's a typo in the css being injected, is there no closing bracket for the :root? I probably did that above too 😅

@rauenzi rauenzi merged commit b62deff into BetterDiscord:main Aug 30, 2023
1 check passed
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

Successfully merging this pull request may close these issues.

[Feature] Add a way for theme to get windows accent colour
2 participants