-
Notifications
You must be signed in to change notification settings - Fork 9
Add theming (WIP) #130
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 theming (WIP) #130
Conversation
Hey! I think we might be solving for a different use case than what our clients need. From what we understand, most clients (like 99%) will want to:
Since AgentPrism gets embedded into existing apps, the host app is usually responsible for things like theme switchers. So runtime theme switching (beyond dark mode) might not be something clients actually need. I went back to the Linear task to check what we originally scoped: Key requirements were:
The expected user flow looked like: /* Client's app.css - set brand colors once */
:root {
--agent-primary: 59 130 246;
--agent-bg: 255 255 255;
}
.dark {
--agent-primary: 96 165 250;
--agent-bg: 17 24 39;
}Just CSS vars defined at build time. Dark mode works via the host app's existing |
mikhin
left a comment
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.
simplifying to focus on build-time CSS var customization
|
|
||
| const filledThemeClasses: Record<ColorVariant, string> = { | ||
| gray: "text-gray-900 bg-gray-100 dark:bg-gray-600 dark:text-gray-200", | ||
| gray: "bg-ap-foreground text-ap-background", |
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.
for text it should be foreground?
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.
Oh, forgot to comment on this - this is not an actual code yet, just a place to test things to see if they work (they do). I'll update this in accordance with a design soon
| prefix: T, | ||
| ): AgentPrismThemePrefixed<T> { | ||
| const prefixedTheme: AgentPrismThemePrefixed<T> = Object.fromEntries( | ||
| Object.entries(themeObject).map(([key, value]) => [ |
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.
Note on all runtime utility functions:
The setTheme(), createTheme(), and theme generation scripts look well-implemented, but I assume these won't be in the final version since they're not relevant to the build-time customization goal from the task.
|
Initially i was trying to kill 2 birds with one stone - both for "static" theming and runtime switching. After reading your comments and giving it a second thought - i agree, we should proceed with the flow which will suit 99% of users, while being reasonably simpler. I'll update the logic soon |
535cd1b to
bd6f6dc
Compare
Still in WIP, need to have a clear look at this before proceeding further.
Adding new theme
Let's say we want to add new theme. For us it means that we need to:
src/themesand fill it with tokens (i used colors from tailwind for 2 existing themes) (AgentPrismThemeinterface)build-theme-files.tsand run it to generate css files for themeConsuming themes in the app
agentPrismTailwindColorsand use it in tailwind config so that tailwind will proceed the classesdefault.css) and add it to yourindex.css- we need this so that some theme will be defined by defaultsetThemeand required theme object, and callsetTheme(selectedTheme)Reasoning
a b cinstead ofrgb()oroklch()? - We need this to support opacity syntax@mikhin what do you think about this? It looks a bit complex, and i will think of a way to bring complexity down