adobe / react-spectrum

A collection of libraries and tools that help you build adaptive, accessible, and robust user experiences.
https://react-spectrum.adobe.com
Apache License 2.0
12.53k stars 1.08k forks source link

Theming isn't really possible using CSS modules #3624

Closed hugomn closed 1 year ago

hugomn commented 1 year ago

๐Ÿ› Bug Report

After spending hours trying to understand what React Spectrum is built to support theming means on the official docs, I couldn't find the proper way to do any customization to a theme, even a very basic border-radius (I don't want to create a custom component, or to style a specific one using the UNSAFE_ property. We have a design system, and I want to make the components consistent without needing extra config per component).

๐Ÿค” Expected Behavior

We should be able to override theme variables or to write a full custom theme overriding the default theme.

๐Ÿ˜ฏ Current Behavior

First of all, the Theming docs mentions vaguely that custom themes can be created, but no examples are provided. Trying to create a custom theme object, and passing some of the properties will override all properties. Copying the full theme file doesn't sound like a viable solution.

๐Ÿ”ฆ Context

I've tried to create a theme object in multiple ways and to pass it to Provider. The code sample below explains the approach and why it doesn't work.

๐Ÿ’ป Code Sample

theme.module.css

.theme {
  /* Button overrides */
  --spectrum-button-primary-border-radius: 0.375rem;
}

index.ts

import customTheme from './theme.module.css';
import dark from './spectrum-dark.module.css';
import darkest from './spectrum-darkest.module.css';
import global from './spectrum-global.module.css';
import large from './spectrum-large.module.css';
import light from './spectrum-light.module.css';
import medium from './spectrum-medium.module.css';

const theme = {
  global: global,
  // MERGING DOESN'T WORK
  dark: { ...dark, ...customTheme },
  // WHEN APPLYING CUSTOM THEME FILE, ALL OTHER STYLES ARE IGNORED
  // dark: customTheme,
  darkest: darkest,
  // MERGING DOESN'T WORK
  light: { ...light, ...customTheme },
  medium: medium,
  large: large,
};

// COMPONENTS VARIABLES ARE NOT INCLUDED IN ANY OF THOSE FILES, I.E spectrum-button-primary-border-radius

export default theme;

๐ŸŒ Your Environment

Next.js 12.2.25 React 18.2.0 @nrwl/next 14.6.1

What am I missing here? Is it the right way to create a theme to duplicate all files from https://github.com/adobe/react-spectrum/tree/main/packages/%40adobe/spectrum-css-temp/vars? Which other option do we have?

Not being able to change a simple border-radius is frustrating. The theming docs also need to be updated to display some examples or to clarify what theming actually means. I've also based my assumptions on this example from the official storybook: https://github.com/adobe/react-spectrum/blob/main/packages/%40react-spectrum/provider/stories/custom-theme.css, which seems to be misleading as well.

snowystinger commented 1 year ago

Closing with https://github.com/adobe/react-spectrum/issues/3370 At this time, this is not possible to support as our token system isn't API. We apologize for any confusion and lost time on this. The best route to custom theming at the moment is to use our hooks and fully control the rendering of any component you want to style differently.

squidjam commented 1 year ago

Closing with #3370 At this time, this is not possible to support as our token system isn't API. We apologize for any confusion and lost time on this. The best route to custom theming at the moment is to use our hooks and fully control the rendering of any component you want to style differently.

Does it mean that we have to reimplement the whole library just to take out round borders from buttons and define some custom colors?

albert-schilling-enerparc commented 1 year ago

I totally understand your point, @snowystinger , that React-Spectrum does not really support customizing themes currently, but that you can basically rebuild it with React-Aria (and React-CSS). But that approach really does not make any sense. Why should one redo all the great work you put into React-Spectrum just to change the values of a color variable? There must be an easy approach to overwriting or adding additional variables to change the theme.

snowystinger commented 1 year ago

Absolutely, that is a great point. We're working on a proposal right now to make that easier. See https://github.com/adobe/react-spectrum/discussions/2368#discussioncomment-3887426

devongovett commented 1 year ago

Well that proposal would still be unstyled @snowystinger. I think they're asking to reuse spectrum's css but with some customizations rather than rewriting it.

snowystinger commented 1 year ago

ah, you are correct, i mis-read that. Your proposal will reduce complexity in terms of getting the react-aria correct with the components. From there, it should be easier to style.

Unfortunately we can't easily expose the css variables of React Spectrum as API for customized theming because they are out of our control and have undergone a few major changes already. If and when they become more stable, we could consider revisiting.

albert-schilling-enerparc commented 1 year ago

Thank you for your reply @snowystinger . This sounds somewhat promising. I have no real knowledge about CSS modules, but wouldn't it be sufficient, if devs could view the css modules with the variable names and values in the docs (autogenerated) and then just add their own css module in the correct order that overwrites these variables? Something along these lines:

import {defaultTheme} from '@adobe/react-spectrum';
import custom-light-theme from '../css/custom-light-theme.css';

<Provider 
  theme={{...defaultTheme
    , light: [defaultTheme.light, custom-light-theme] }
>
  <YourApp />
</Provider>

Now, it should be sufficient, that React-Spectrum imports the css-module in the correct order as placed in the array, shouldn't it?

devongovett commented 1 year ago

Yeah the main problem is actually stabilizing the variable names themselves. So far they have been an implementation detail, and the names have changed several times. If we wanted to officially support this, we'd need to treat them as public API that followed semver.

albert-schilling-enerparc commented 1 year ago

Ah, I see. That gives hope that this is only a matter of time. I hope you come up with good future-proof var names and get rid of the cumbersome spectrum prefix :) I think there is some nice inspiration out there regarding naming like the tokens from the IMB Carbon System or the utility classes from Tailwind CSS.

squidjam commented 1 year ago

Maybe if customizing was left to the user by having undefined vars in the css of the elements and all your current values as fallbacks? You are already kind of doing it, so is something like this possible?

background-color:var(---spectrum-custom-background-color-transparent, var(--spectrum-fieldbutton-quiet-background-color-disabled,var(--spectrum-alias-background-color-transparent)))

Variable names can still be changed on your side, while we would be still be able to customize things by keeping the custom ones more or less static.

snowystinger commented 1 year ago

I agree that things could be better. I'm not sure we currently have the bandwidth to implement such a system at the moment.

I think there are a lot of questions about what would be supported in terms of variables. Would it only be colors? or would it also be dimensions? What about in cases of shorthand properties? or positioning or psuedo elements or properties we don't use? or places where we use the same value, but someone customizing would want two separate values?

I'm also not so sure that allowing them to be component specific is the best idea. Our color scale is specifically built to meet contrast a11y requirements and all our components work together. That said, it's a big undertaking to make a new color scale if you're a third party and you just want to tweak a couple of things and are aware of the dangers.

All I'm really saying is that this is something that isn't simple. This would be a huge API surface area. It needs time and people to think through, make a proposal, implement, document, and maintain.

What you've outlined is a good starting place. I think starting from the Tailwind angle could also be worthwhile.

squidjam commented 1 year ago

I agree that things could be better. I'm not sure we currently have the bandwidth to implement such a system at the moment.

I think there are a lot of questions about what would be supported in terms of variables. Would it only be colors? or would it also be dimensions? What about in cases of shorthand properties? or positioning or psuedo elements or properties we don't use? or places where we use the same value, but someone customizing would want two separate values?

I'm also not so sure that allowing them to be component specific is the best idea. Our color scale is specifically built to meet contrast a11y requirements and all our components work together. That said, it's a big undertaking to make a new color scale if you're a third party and you just want to tweak a couple of things and are aware of the dangers.

All I'm really saying is that this is something that isn't simple. This would be a huge API surface area. It needs time and people to think through, make a proposal, implement, document, and maintain.

What you've outlined is a good starting place. I think starting from the Tailwind angle could also be worthwhile.

Making it all optionally customizable for the user could be interesting. Not easy or fast, but interesting.

Now, if this is not the vision of this implementation, maybe stating it clearly how strict this implementation is in the documentation would be a great time saver.

P.S. Your color palettes won't match corporative branding colors, so doing it at the component level would allow someone to control this without redefining any palette you provide. P.P.S Going the Tailwind way seems to me like more work, and might not allow users to customize the design system implementation with their own style requirements.

albert-schilling-enerparc commented 1 year ago

@snowystinger Thank you for your explanation. Your points are absolutely valid. It is definitely the right way to have a clear concept regarding feature set, implementation and maintenance.

On the other hand, I think it is sometimes smarter to start the conceptual work simultaneously with the implementation and improve both incrementally. For example, you could start with a very small API offering only the most important feature for customers, let's say for example adding new color variants. This would offer your users a huge benefit and your team a lot of insights regarding further steps for the implementation and overall concept.

It might also help in evaluating reasonable css variable names.

(Just a side note regarding implementation, maybe an npx script that properly modifies or appends to the existing CSS modules according to the package version might be a first foot in the door. The script might also warn or throw an error if the reference color by the user is not suitable for a color scale.)

devongovett commented 1 year ago

I don't think we really want to support customizing Spectrum itself. Spectrum is our design system for internal Adobe applications, and for external developers integrating with Adobe software. In both cases, we want the design to match other Adobe products. Allowing customization would open this up and defeat the purpose of having a design system. It would also make it harder for us to change the design and implementation without breaking things in the future, because we'd have to keep all of the potential customizations in mind.

I think what we can offer instead is a starter kit, with examples of all of the components built with React Aria with custom CSS and JS that you could modify. We could build it with some CSS variables that you could customize globally, or you could just modify the CSS to make more detailed changes. This way, we don't need to worry about breaking things in future updates, and you get full freedom to to whatever you want while starting with something pre-built rather than from scratch. You could technically do this now by forking the repo and changing the CSS, but there's a lot here and I think we can make it easier.

hugomn commented 1 year ago

Spectrum is our design system for internal Adobe applications, and for external developers integrating with Adobe software.

If that's the case I really insist that the documentation should be heavily changed. As a first-timer getting to the docs, it gives a clear impression that React-Spectrum is an open react library that could be used in any application and customized properly. First, the statement "Spectrum is a design system for internal Adobe applications, and for external developers integrating with Adobe software" should be in the main page of the docs, and on the theming section, this needs be removed: "React Spectrum is built to support theming. Colors, sizing, and spacing options can be customized through the use of CSS variables which are defined using the Provider component."

Sorry to insist on this, but I want to avoid people spending hours investigating and doing proofs-of-concept with Spectrum to hit a wall down the road as I did. ๐Ÿ™Œ๐Ÿผ

albert-schilling-enerparc commented 1 year ago

Spectrum is our design system for internal Adobe applications, and for external developers integrating with Adobe software.

If that's the case, then I would be very sad. I just hate the thought to reinvent the wheel and to recreate a design system each time I implement a new frontend. The Adobe Design System is specifically suitable for data visualization / industry apps in my opinion. Many other design systems seem to focus more on B2C apps. I hope Adobe sees the benefit in offering its Design System as a piece of open software.

devongovett commented 1 year ago

We can adjust the wording a bit but I don't think it's completely wrong. The main page says "A React implementation of Spectrum, Adobeโ€™s design system." - I'm not sure where you got the idea that you could totally change the design of it. What we mean by theming is as described on the Spectrum design website โ€“ Spectrum itself has multiple themes, both in terms of color schemes (dark/light) and designs for different adobe products (like CC express). Other themes are possible if you want to take a look at the builtin themes and write something similar, but we wouldn't recommend it as that would go against the Spectrum design guidelines. That's where React Aria comes in. I think we can offer something in the middle for more easily building custom design systems.

hugomn commented 1 year ago

I'm not sure where you got the idea that you could totally change the design of it

I got this idea from this statement, inside the theming page:

React Spectrum is built to support theming. Colors, sizing, and spacing options can be customized through the use of CSS variables which are defined using the Provider component.

The way you describe it is far from reflecting that statement. It should be something like this: "Customizing React Spectrum is not possible, unless you create a new theme based on the existing ones and rebuild it, which is not recommended".

The "How themes are defined" section gives me a clear idea that I would just need to override defaultTheme and pass it to Provider, which is very misleading. That section should be called "How to switch to one of the existing themes" and another section called "How to create a custom theme" should be created saying explicitly that this is only possible by extending the code somehow. Sorry to be nit-picky here, but this is what you would see in any open-source UI library.

devongovett commented 1 year ago

I think there's a difference between theming and customization. We're describing how the themes work, not how to make a custom one. "How themes are defined" lists the three themes we provide, describes how they work, and shows how to apply them. It doesn't show any CSS or examples about how to make your own theme. I agree we can clarify that custom themes aren't supported, but I don't think it's misleading to simply describe how the theme system works.

Sorry to be nit-picky here, but this is what you would see in any open-source UI library.

I disagree. There are tons of open source design systems out there that don't support customization. The point of a design system (vs a component library) is enforcing design consistency between products. So at that layer, that is our goal. We have additionally gone above and beyond to provide a lower level API in React Aria that supports full customization beyond what most libraries offer. React Spectrum is our implementation on top of this foundation for our products and anyone else who might want to use it. If you don't want to use our design, then you can make your own design system with this foundation. We're always looking for ways to make this easier, but customizing our design is not the way we'd like to go. We'd rather approach it from the opposite direction.

squidjam commented 1 year ago

I'm not sure where you got the idea that you could totally change the design of it.

Being able to change the border radius of a button doesn't seem like a lot.

We can adjust the wording a bit but I don't think it's completely wrong. The main page says "A React implementation of Spectrum, Adobeโ€™s design system."

We're going into semantics here. Maybe just take the recommendation of clarifying the scope of the implementation. This could save a lot of time and effort from others looking for a base on which to build upon for their own purposes.

I'm not sure where you got the idea that you could totally change the design of it.

Well, the complete quote on this (link) says: "A collection of libraries and tools that help you build adaptive, accessible, and robust user experiences." .

Add to it this little tidbit from Spectrum's blog (link):

We believe there is an opportunity to share much of the behavior and component logic between design systems and across platforms. For example, user interactions, accessibility, internationalization, and behavior can be reused, while allowing custom styling and rendering to live within individual design systems. This has the potential to improve the overall quality of applications, while saving companies money and time, and reducing duplicated effort across the industry.

All I am asking for is to please state explicitly in the documentation: what's the scope of the React implementation and who it is intended for.

devongovett commented 1 year ago

Yes, the homepage lists three different libraries and describes what they are for. The description of React Spectrum says

A React implementation of Spectrum, Adobeโ€™s design system. Spectrum provides adaptive, accessible, and cohesive experiences for all Adobe applications.

I think this is pretty clear.

The blog post is describing React Aria, which implements reusable "user interactions, accessibility, internationalization, and behavior". It is a part of our overall architecture.

I already said we would clarify the description even more but I think this has now been blown way out of proportion. I understand you're frustrated that you can't customize at the React Spectrum layer, and I'm sorry, but that's not going to change. We are focused on providing tools for building your own design system instead, and will continue to make that easier.

squidjam commented 1 year ago

Yeah, good luck