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

feat(store-ui): Basic CSS theme and theme-addon for Storybook #828

Merged
merged 17 commits into from
Aug 13, 2021

Conversation

Gmantiqueira
Copy link
Contributor

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

@Gmantiqueira Gmantiqueira requested a review from a team as a code owner July 20, 2021 21:56
@netlify
Copy link

netlify bot commented Jul 20, 2021

✔️ 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/

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 20, 2021

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:

Sandbox Source
Store UI Typescript Configuration

Copy link
Contributor

@tlgimenes tlgimenes left a 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

@Gmantiqueira
Copy link
Contributor Author

I'm not sure if this was intended, but when switching themes they eventually stop working

Thanks @tlgimenes, I think it's working now.

Copy link
Contributor

@tlgimenes tlgimenes left a 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.

@@ -0,0 +1,11 @@
# `basic-theme`
Copy link
Contributor

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

@Gmantiqueira
Copy link
Contributor Author

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.

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.

@tlgimenes
Copy link
Contributor

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.

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.

prefixing is better because they stay together when listing the folders

Copy link
Contributor

@icazevedo icazevedo left a 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.

packages/store-ui/.storybook/preview.js Outdated Show resolved Hide resolved
@Gmantiqueira
Copy link
Contributor Author

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 theme-default because I'm just copying common elements from storecomponents. Any thoughts about another name? theme-store? 🤣 just kidding

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, theme-b2c, theme-fashion, theme-b2b. I like the idea of doing those themes with the "visual point of view", but we need a focused designer on this, a structured design system about and blablabla 🤔 But we can take this idea forward.

@icazevedo
Copy link
Contributor

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 theme-default because I'm just copying common elements from storecomponents. Any thoughts about another name? theme-store? 🤣 just kidding

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, theme-b2c, theme-fashion, theme-b2b. I like the idea of doing those themes with the "visual point of view", but we need a focused designer on this, a structured design system about and blablabla 🤔 But we can take this idea forward.

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 theme-b2c-basic for now, but that's just a suggestion :).

@Gmantiqueira Gmantiqueira force-pushed the themes/basic-theme branch 2 times, most recently from 66cfc26 to 16f5feb Compare August 13, 2021 17:21
@Gmantiqueira Gmantiqueira merged commit e7338ad into master Aug 13, 2021
@Gmantiqueira Gmantiqueira deleted the themes/basic-theme branch August 13, 2021 17:37
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.

4 participants