-
Notifications
You must be signed in to change notification settings - Fork 63
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
feat(store-ui): Basic CSS theme and theme-addon for Storybook #828
Conversation
✔️ Deploy Preview for storeui ready! 🔨 Explore the source changes: 16f5feb 🔍 Inspect the deploy log: https://app.netlify.com/sites/storeui/deploys/6116aa2d9075e80007cf5ad4 😎 Browse the preview: https://deploy-preview-828--storeui.netlify.app/ |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 16f5feb:
|
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'm not sure if this was intended, but when switching themes they eventually stop working
be79b80
to
5dae769
Compare
Thanks @tlgimenes, I think it's working now. |
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 it's ok. I'm just not sure about the theme name itself. My two cents is that we should prefix all themes with theme and then the name. I'd name something like:
theme-default
theme-default-dark
.
packages/basic-theme/README.md
Outdated
@@ -0,0 +1,11 @@ | |||
# `basic-theme` |
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.
Also, don't forget to add something to the README.md
I was thinking about putting the "theme" as a suffix instead of prefix, but as a prefix makes it more clear. Nice, I'll change. |
6578d03
to
0dc23b7
Compare
prefixing is better because they stay together when listing the folders |
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.
The deploys failed so I can't see the changes visually 😢
Also, have you thought of naming the theme differently? Personally, I don't like naming things like "default" or "base" because it doesn't convey the real meaning/purpose of the thing, or what the thing is (a little bit philosophical).
I'd really love if we could give personality to our themes' names, like: theme-boring
(what I imagine a really clean theme would be hahahah), theme-night-sky
, theme-good-morning
, theme-rainbow
and etc.
@icazevedo Well, I didn't think of something specific about this About those themes you mentioned, I'm not sure, I don't think if this is what we want for these themes. Like, yes, we need something more clear about our themes, but I think we need to name them from the "business point of view". For example, |
I totally agree with your points. The "business point of view" is very understandable but is not fun hahahaha. Just something to think about. Maybe we could call it |
993324b
to
ffd348f
Compare
1890265
to
21eed7b
Compare
Co-authored-by: Ícaro Azevedo <icaro.azevedo@vtex.com.br>
66cfc26
to
16f5feb
Compare
How it works?
This PR creates a package with a simple TailwindCSS for our Storybook.
Also adds a feature to switch between themes inside our Storybook.
How to test it?
Storybook preview.
References
Storybook Addon Themes