-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Theme color direct access #105296
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
base: master
Are you sure you want to change the base?
Theme color direct access #105296
Conversation
Co-authored-by: jonas.badalic <[email protected]>
|
Cursor Agent can help with this pull request. Just |
static/app/views/performance/newTraceDetails/traceDrawer/details/styles.tsx
Show resolved
Hide resolved
| aliases: Aliases | ||
| ) => ({ | ||
| tooltipUnderline: (underlineColor: ColorOrAlias = 'gray300') => ({ | ||
| tooltipUnderline: (underlineColor: ColorOrAlias | string = 'gray300') => ({ |
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.
@JonasBa there’s only a handful of usages that use tooltipUnderline with a specific color, and it’s only:
linkUnderlinegray200backgroundsuccesssuccessText,warningText,errorTextgreen300,red300,gray300
I think we should be able to find token replacements for those and just use our color tokens here?
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.
iirc there was another component that was using this, so I wanted to keep the changes small, but let me see if we can indeed one shot this
The ColorOrAlias type is problematic since it creates a coupling between how the theme is structured and which colors are available. Along the same lines, organizing our theme with a flat key list isn't a scalable long term approach.
Developers should use variants to encapsulate different color meanings or access colors directly on the theme.