-
Notifications
You must be signed in to change notification settings - Fork 26
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
skia token name enum can extend brave chromium enum #343
base: main
Are you sure you want to change the base?
Conversation
👋 Thanks for Submitting! This PR is available for preview at the link below. ✅ PR tip preview: https://343.pr.leo.bravesoftware.com/ Variables Diff--- ./tokens/css/variables.old.css 2023-07-26 18:56:54.244941229 +0000
+++ ./tokens/css/variables.css 2023-07-26 18:56:23.857379450 +0000
@@ -1,6 +1,6 @@
/**
* Do not edit directly
- * Generated on Tue Jul 25 2023 06:44:49 GMT+0000 (Coordinated Universal Time)
+ * Generated on Wed Jul 26 2023 18:56:23 GMT+0000 (Coordinated Universal Time)
*/
:root {
|
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 LGTM but lets rope in a C++ expert.
enum class Color : ui::ColorId { | ||
kLeoColorsStart = kBraveColorsEnd, | ||
<%= groupedTokens.light.allTokens.map(prop => ` ${prop.name}`).join(',\n') %>, | ||
kLeoColorsEnd | ||
}; |
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.
How about wrapping these E_CPONLY() and define macro so that we can add this to brave_color_ids?
Roughly,
enum class Color : ui::ColorId { | |
kLeoColorsStart = kBraveColorsEnd, | |
<%= groupedTokens.light.allTokens.map(prop => ` ${prop.name}`).join(',\n') %>, | |
kLeoColorsEnd | |
}; | |
#define LEO_COLOR_IDS \ | |
<%= groupedTokens.light.allTokens.map(prop => ` E_CPONLY(${prop.name}) \ | |
`).join(',\n') %>, | |
}; |
and in brave_colod_id
#define BRAVE_COLOR_IDS \
BRAVE_COMMON_COLOR_IDS \
BRAVE_SEARCH_CONVERSION_COLOR_IDS \
BRAVE_SIDEBAR_COLOR_IDS \
BRAVE_SPEEDREADER_COLOR_IDS \
BRAVE_VPN_COLOR_IDS \
BRAVE_VERTICAL_TAB_COLOR_IDS \
- BRAVE_PLAYLIST_COLOR_IDS
+ BRAVE_PLAYLIST_COLOR_IDS \
+ LEO_COLOR_IDS
};
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 wish we have leo_color_mixer generated too 😄
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 think @petemill has a solution for that!
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.
@sangwoo108 That was the first strategy I tried for this PR, but I got some errors. And ultimately it seemed ok for it to be a separate enum set, similar to how chromium does it with separate enum sets. I'll try again to see what the error is. In the meantime, here's the LeoColorMixer PR https://github.com/brave/brave-core/pull/19440/files
Modifies the skia enum so that it can be used in the brave/chromium color mixer. I didn't worry too much about making it non-chromium (and non-brave) compatible since there are no uses outside that at the moment.
Corresponds with brave/brave-core#19440