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

[Admin] Use generic color class names for Tailwind theme colors #5848

Open
tvdeyen opened this issue Sep 4, 2024 · 7 comments
Open

[Admin] Use generic color class names for Tailwind theme colors #5848

tvdeyen opened this issue Sep 4, 2024 · 7 comments

Comments

@tvdeyen
Copy link
Member

tvdeyen commented Sep 4, 2024

Desired Behavior

I would like to be able to theme the Solidus admin as I am currently able now with the legacy backend. There we have bootstrap variables that can be changed to new color values. The use of generic variables (primary, secondary, danger, brand, etc.) makes it very easy to change the look of Solidus admin. Also light and darkmode are easier to maintain that way.

Current Behavior

We currently have color based class names all over the new admin. This makes it very hard to change the look of the new admin. Also it makes it much harder for us to change the theme in the future and make adjustments to the light and dark modes.

Additional notes

Solidus is a framework that allows clients to adopt the admin to their needs. This includes theming. IMO this is one of reasons for the success of Solidus in the community. We should not remove this feature that many stores rely on.

In the tailwind docs are ways described to make this more maintainable for us. https://tailwindcss.com/docs/customizing-colors#naming-your-colors

I think we should move to a more framework-alike approach with the new admin.

@tvdeyen
Copy link
Member Author

tvdeyen commented Sep 4, 2024

@solidusio/core-team I would like to here your thoughts on this. Is theme-ability a feature we want to keep or do we just want Solidus to always look as is? I am open to change this, but think we should at least talk about it publicly and tell the community what we are up to with the new admin.

@kennyadsl
Copy link
Member

@rainerdema @elia, do you remember why we used this approach? Did we ever think about prioritizing theming before?

@elia
Copy link
Member

elia commented Sep 4, 2024

@kennyadsl @tvdeyen yes, we just stuck with the TailswindCSS recommendation, additionally it was no time to devise a theming system, which is essentially another API to design, maintain, and migrate.

Sounds like we might have a really good reason to steer in that direction now, as the documentation says:

Again, we recommend sticking to the default naming convention for most projects, and only using abstract names if you have a really good reason.

That said nailing the right level of abstraction with those colors is really hard, I tried it many times, and it will make interpreting tailwind classes harder (bg-primary-intense is not as clear as bg-gray-900) and would mean thinking about the role of each color, having multiple roles target the same colors and a bunch of other puzzles that are not straightforward to solve.

So I propose to limit the customization to something like the accent color, and go vanilla with the general design, I might be wrong, but that covers the need to match the style of a client in most cases.

@tvdeyen would that cover your use-case? How deep you think the customization would need to be?

@tvdeyen
Copy link
Member Author

tvdeyen commented Sep 4, 2024

@kennyadsl @tvdeyen yes, we just stuck with the TailswindCSS recommendation, additionally it was no time to devise a theming system, which is essentially another API to design, maintain, and migrate.

Sounds like we might have a really good reason to steer in that direction now, as the documentation says:

Again, we recommend sticking to the default naming convention for most projects, and only using abstract names if you have a really good reason.

That said nailing the right level of abstraction with those colors is really hard, I tried it many times, and it will make interpreting tailwind classes harder (bg-primary-intense is not as clear as bg-gray-900) and would mean thinking about the role of each color, having multiple roles target the same colors and a bunch of other puzzles that are not straightforward to solve.

So I propose to limit the customization to something like the accent color, and go vanilla with the general design, I might be wrong, but that covers the need to match the style of a client in most cases.

I agree. Limiting it to things like brand-color and maybe some state colors (danger/error, info, success/notice, warning/alert) should be enough to satisfy customization needs of stores.

@tvdeyen would that cover your use-case? How deep you think the customization would need to be?

Yes, that will cover it and I think the customization level should be as shallow as possible. Just cover the basic needs.

One idea would be to read a solidus/tailwind.config.js file from stores (and potentially extensions) with the default Tailwind config from admin exported as module.

@fthobe
Copy link
Contributor

fthobe commented Dec 11, 2024

Are branding features a topic only for the backend or is it something that you want to consider for starter front end as well?

@tvdeyen
Copy link
Member Author

tvdeyen commented Dec 11, 2024

Are branding features a topic only for the backend or is it something that you want to consider for starter front end as well?

Since the starter frontend generates a dedicated Rails app for each store, branding happens in that Rails app, not in Solidus.

@fthobe
Copy link
Contributor

fthobe commented Dec 11, 2024

Seems fair.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

No branches or pull requests

4 participants