-
Notifications
You must be signed in to change notification settings - Fork 4
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
Feedback #1
Comments
I've been working on an internal lib at work for awhile now and came to many similar conclusions. Agree 100% on classnames as the base for styles. Just makes the most sense, as its the most generic/least specific, and should work well with solutions like react-themeable. The only problem I've been about to think of is that any change in class name is a breaking change, but thats obvious. I've been tinkering around with an idea to create a "deprecated soon" warning for css classnames being use on a page that will soon be changed (think react's console warning messages each time you upgrade react, but for css classnames). React-themeable looks good also, though I've been have trouble wrapping my head using Jed Watson's ClassNames repo with it and without using JSX, but I'm sure that a failing on my part and not react-themeable. Agree on self contained. Shipping without a theme...I think the core lib should have as minimal styles as possible, but there are a few components that just need styling in them even at a base level. Think of things like Date Pickers and whatnot. If the lib is using a theme utility though, a must would be to have a default theme in that utility that ships with the lib. I think most people won't create their own theme from scratch, but rather build on top of/hack up whatever theme options are given to them by the lib author. Oleg's number 5 is a good point. Which could be controlled with a theme component. Just restrict specifically what props/styles the theme component uses. |
super invaluable feedback @kof @NogsMPLS I like the idea of warning users for breaking changes in classnames. There could be utility function (only running in dev mode) which logs out errors in case the provided theme or global theme uses a certain function. Very much like React's dev experience. @NogsMPLS I agree that a default theme should come with the library. I was thinking about dev have to make one import+function call to inject it. Thoughts? I like the idea about number 5, but couldn't wrap my head around how this should work. When we shipped the initial version of Belle it came with a default theme + a Bootstrap theme. The same components were so different and hard to imagine what other themes might change or have in common. One experiment could be to create 4-5 themes and then see what they have in common. |
Regarding number 5: maybe a theme should consist of 2 parts: classes and options. Options contains all constants needed to configure a component to work well with given classes. In case of JSS, I would create a stylesheet which would use the constants to create the JSON. And those constants will be passed to the component as a part of the theme, so that they can be used for calculations. |
I like that @kof, that sounds reasonable. Provide a default theme, but make it easily overridable. Maybe have the default theme in the component itself, and make it customizable via props? |
4-6-7 - not sure right now. |
Although I agree on all the points, I think that consumers (i.e. other UI developers) do not really care if some UI lib uses classes or inline styles underneath as long as you can override the look and feel of it easily. The good news is projects like https://github.com/markdalgleish/react-themeable or https://github.com/andreypopp/rethemeable allow using any available method of styling components. I think this is really important and should not be discouraged because of personal preferences. I'm not a fan of inline styles myself, but saying all libs should follow one approach will alienate it for some developers. I really like the approach of VicotryJS in describing how to write components using its architecture: https://github.com/FormidableLabs/builder-victory-component/blob/master/dev/CONTRIBUTING.md#component-style To me, the ultimate goal of such project would be an API description + a set of tools (docgen, linters, etc) to allow other UI-lib authors integrate into their existing projects with ease. It seems there are too many different UI-libs for React being developed ATM and it's almost impossible to combine or exchange them without rewriting lots of own code. Regarding themes: I think the most idiomatic way to do would |
also it's good idea to separate validation. E.g. at my current project i'm using: https://github.com/davidkpiano/react-redux-form |
After letting your feedback sink in for a little while in addition to talking to some people I figured this Factory Pattern approach might do the job. @kof the factory function to create a theme element also contains @okonet love you idea with the linters. this we we could ensure certain quality & a good style. Regarding point 5 (theme vs core style). I think this then could be an implementation detail of a styled UI kit package which the core (unstyled) components don't have any related code. I'm curious why you think 😄 |
@andreypopp I wonder what you think about it … |
@nikgraf 👍 I love that, especially with CSS Modules, i.e. const customTheme = {
questionMark: styles.questionMark,
visibleContent: styles.visibleContent,
}; |
@mxstbr in this case you don't even have to do the mapping. You can directly pass the styles into the theme. Checkout https://github.com/nikgraf/future-react-ui/blob/master/app/components/HintCssModulesExample/index.js |
For our internal UI component library we use an approach described as React Stylesheet. Actually this conceptually similar to factory pattern described by @nikgraf but with more convention around how factory is implemented and how mechanism for theme overrides works. The idea is that a component which is meant to be styled in a few different ways define a stylesheet as a mapping from names to React components, for example: class Button extends React.Component {
static stylesheet = {
Root: 'button',
Caption: 'div',
}
render() {
let {caption} = this.props
let {Root, Icon} = this.constructor.stylesheet
return (
<Root>
<Caption>{caption}</Caption>
</Root>
)
}
} So that This way we are agnostic of what underlying mechanism is used for styling DOM elements. Then React Stylesheet defines a function which allows to derive a styled component from an original one. For example if we want to style out import {style} from 'react-stylesheet'
let SuccessButton = style(Button, {
Root(props) {
return <button {...props} className="ui-Button--success" />
},
Caption(props) {
return <div {...props} className="ui-Button__caption--success" />
}
}) This works with CSS Modules and inline styles as well, with JSS or good old SASS. Nested overridesThe mechanics is a little more sophisticated though, the What if you have a class Dialog extends React.Component {
static stylesheet = {
Root: 'div',
Button: Button, // defined above
}
render() {
let {Root, Button} = this.constructor.stylesheet
return <Root>{children}<Button /></Root>
}
} Now let StyledDialog = style(Dialog, {
Button: {
Root: (props) => <button {...props} style={{background: 'red'}} />
}
}) Now Granular styling APIComponents can define a granular interface for styling themselves by defining class Button extends React.Component {
static stylesheet = {
Root: 'button',
Caption: 'div',
Icon: null
}
static style({icon, ...override}) {
// allow easy icon override
return style(this, {
Icon: (props) => <span {...props} className={`glyphicon-${icon}`} />,
...override
})
}
render() {
let {Root, Icon, Caption} = this.props
return ...
}
} Then let ButtonWithMakeIcon = style(Button, {icon: 'plus'}) |
Regarding propagating theme through the React's context mechanism. Name-clashing. It can be solved by using not string keys but class Button extends React.Component {
static themeKey = Symbol('Button.themeKey')
render() {
// resolve theme using `this.constructor.themeKey` from context.
}
} Then let Theme = {
[Button.themeKey]: {
// theme for Button
}
} That's how Rethemeable lib works. But we found that we usually don't want to use context for theming as we don't have such requirements as switching themes in an app via some sort of theme switcher. Instead we use React Stylesheet to predefine themed components for some particular app and then use them directly. |
As for DOM styling mechanism we use a CSS in JS approach. React Stylesheet allows to configure fallback for DOM components so we style DOM components directly when styling composite components. The example with import {style} from 'react-stylesheet'
import {style as styleDOM} from 'react-don-stylesheet'
let SuccessButton = style(Button, {
Root: { // this calls into `styleDOM` actually as `Root` is defined as `button` DOM component
backgroundColor: 'green',
...
}
}, {styleDOM: styleDOM}) |
The unstyled lib + theme ui kit idea is interesting, because it seems like the consensus would be that we end up with 3 "types" of components. I'm going to take some terminology for this article: https://medium.com/@dan_abramov/smart-and-dumb-components-7ca2f9a7c7d0#.pbtswjdvc: We would have a Presentational/Dumb Component for markup, a Styled Component that sets base styles and accepts themeing props, and a Container/Smart component in an actual App to handle state and business logic. Which is interesting, because now it starts to look very much like the old school 'separation of concerns' pattern where we keep markup, styles and logic all separated, but still all inside React. |
+1 on using Symbol when using context ( @andreypopp ) At the end, both approaches mean that receiver component needs to take a theme and theme options via props or context. We can provide a function implemented by react-themeable which will pick the theme from props first, if not found from context. This way component creators don't need to care about how to get the theme. A theme for one component should bring a map of classes and options object. This will allow to have static cachable styles as well as options to configure component to match those styles. |
The need for both context and props theme passing I can see: flexibility to change a theme for a specific component at any node in react's render tree.
|
I'm a bit worried/scared of introducing context for theming. If we solely rely on props then components theming has this functional character: "calling a function f twice with the same value for an argument x will produce the same result f(x) each time". Context behave like side-effects influencing the styling and also a bit unpredictable from where things are coming. In addition this opens up the question if we merge the theme or overwrite the whole thing. In addition I wonder how much value it has to theme a whole section differently. Applying a few theme props might do the job as well or do I miss real world use-cases (that I haven't experienced yet) Thoughts? |
How about use custom-tag and structure of the selector to define style:
Component is the basic semantic unit, we do not care the internal implementation, so we do not need class attribute in inner component. Using structure of the selector can make html structure first. If you can not define your style clearly, maybe you need to split some children components. |
@ustccjw can you provide an example? (would help a lot) |
@nikgraf regarding context, I understand the fear that its possible to redefine theme at any point via context and it might be hard to find out where. For this case we can have a restriction that a theme over context for the same components set, can be defined only once. Once its defined, overwrites over context are not allowed, do it via props. So you can be sure where to find it. I think the Idea for using context is all about to provide a great UX from the beginning with a minimal setup. |
I have a feeling that contexts suits as well for theming as it suites great for localization and I just don't get a fear that a lot of people have about them. |
@Lapanoid the question is what context gives you for theming what cannot be implementing using props? |
@andreypopp simplicity of the setup. You can wrap an entire application into one Theme component. |
@kof I feel like this is only theoretically simpler. In practice you'd need to worry you have enough things configurable/styled via context vs. props. |
@andreypopp Basically to theme whatever Component I want in any position in the tree.
I need to pass props here through |
More info can check: https://github.com/ustccjw/tech-blog/blob/master/client%2Fcomponent%2Farticle-list%2Findex.jsx |
@Lapanoid my assumption is that you don't need to theme a component tree in an app but build one using themed components. |
@andreypopp I would probably do that too, however building simple things would feel like big overhead ... |
@ustccjw this is a very opinionated solution |
@andreypopp my assumption that theming should be centralized on app level, that does not mean that I want to change it dynamically. |
One of the basic ideas @nikgraf outlined was a Global Theming Utility. I really like this idea and think it's important for adoption. I think the context idea laid out by @kof seems like a decent way to do this. Shipping themed components is great for a 'base theme' for any given library, but I'm having trouble on figuring out how to change specific theme aspects without having to define a custom theme for every view. To help explain what I mean: //import a base Styled Success Button from some lib Theme UI Kit
import SuccessButton from 'someLib/SuccessButton';
//provide a custom theme for this view
const customTheme = {
background: 'custom-green-class',
};
export default (props) => {
return (
<div>
{/* Default themed component from Library Theme UI Kit */}
<SuccessButton />
{/* Overwriting the theme locally for this case */}
<SuccessButton theme={customTheme}/>
</div>
);
}; The above illustrates someone that wants to use some library's UI Theme Kit Success Button, but the default green doesn't match their brand, so they override the second button with new The capability to do this is great, but I'm wondering with this pattern - that without a context implementation like @kof suggested - how would I change ALL SuccessButtons ever used in my app to use How do people change all SuccessButtons at the same time?
Most people don't use libraries exactly how they come out-of-the-box and will want to change things easily, and probably globally. That's actually one idea I really like with current Belle's configuration options. It's not classname based, but being able to just glance at that outline, and easily globally override all card styles really adds a lot of immediate value in my eyes. ..Or perhaps my thinking on this subject is too old school and influenced by old css non-component theme override thoughts. Maybe forking a UI Kit for any given project to create your own styled components really is the best way to handle this. |
UI libraries shouldn't have to concern themselves with styling strategies or rebuilding common primitives. And their UI components should not be handling styles different to those from components exported by other libraries or defined in the app. Therefore, I'd suggest UI libraries exporting React components eventually want to be built upon common primitives and APIs, like those React Native provides. This is what I started react-native-web for at Twitter. |
Yes, well, common primitives for styling on the web is basically class names. |
What makes you think that? |
.. we are stepping at the flaming ground .. |
Well classes is not the only way, there are also lots of other selectors. But what do you use most of the time? Sounds like a common primitive if I understand correctly @necolas. |
Classes is perfect because they are not opinionated about how static CSS rules have to be created. |
By 'primitives' I meant UI components like |
@kof Compared with css in js,i do not think this solution is very opinionated. |
@Lapanoid I have 3 issues with context that come to my mind. Let's look at this example structure: App.js
Navigation.js
Main.js
The issues I see here:
Let's see how a factory approach would do in comparison: App.js
Link.js
Navigation.js
DashBoardButton.js
Main.js
Unfortunately more lines of code … I wonder if this is more sane or just annoying overhead to you and how limiting it is for developer experience. /cc @kof @andreypopp |
@necolas Cool stuff @ react-web-native. I like the idea! I agree with you on primitives. Though this proposal is targeted at UI libraries more for Ratings, Toogles, Datepickers or Tag Inputs. I think one of the most undervalued effect of React is the encapsulation React provides. You can have great UX for Desktop, Mobile & even Screen readers encapsulated in these components and just drop them into your App. I believe components should be available unstyled so people can apply their own branding. Still for prototyping or building internal tools you often want to have an already styled version of all these components to move fast. I think Bootstrap showed us the way here :) |
@NogsMPLS I think a lot about the concerns you mentioned. That's why we introduced the global theming for Belle. I also thought about doing it in a similar why in this experiment, but in the end it was a trick/workaround leveraging references. To me it felt people need to know a lot about JS references to avoid messing up here. https://github.com/nikgraf/future-react-ui/blob/master/ui-lib/StaticProperty/Hint.js How do you feel about the example above? Is it that bad? We maybe could build a helper function that generates your own themed UI lib with the factory approach:
@NogsMPLS does this qualify as understandable & useful |
@nikgraf we could also go with factory function first and keep the context option in mind for later. |
+1 to @necolas's concept of building up of primitives, if I'm understanding it right. For example: const MyButton = factory(Button, {
classNames: { ... },
styles: { ... },
})
const MyDropdown = factory(Dropdown, {
classNames: { ... },
styles: { ... },
components: {
Button: MyButton,
}
}) And then the functionality that lets you compose child Using the primitives over just From what I've learned so far, I currently see "themes" as being made up of:
For Variants could be then converted to being as simple as: <Button primary danger /> But instead of being hard-coded into the base UI Kit, they are able to be defined as whatever by the developer who builds the theme. |
@ianstormtaylor I ended up with theme-standard which is essentially {...options, classes, styles, themes} = theme, I like your |
@kof nice! I think after adding variants, the only thing missing is the ability to compose actual components instead of just being able to style them with Passing in |
class names based only +1
ui lib should ship without a theme +1
ui components should be requirable separately (we often just need the f..g button))))) +1
react-themeable +1
We need to distinguish between a theme and system styles. While changing colors, background images and rounded corners should not have much impact on transitions, changing sizes and margins will. There is a need for clear definition of what is a theme so that it doesn't breaks components.
How to pass the theme ... ? We can support 2 approaches at the same time.
In user land you import your themed button:
import Button from '../ui/Button'
theme
over context can be supported by the UI lib. This is a dirtier but faster way. It can be used as a fallback if notheme
prop has been passed. A ui lib name for e.g. "elemental" should be used as a convention for the namespace on the context object to avoid collisions.I am against using static props/class props. They don't play nicely with higher order components.
The text was updated successfully, but these errors were encountered: