input-output-hk / react-polymorph

React components with highly customizable logic, markup and styles.
Apache License 2.0
79 stars 18 forks source link

Removing react-css-themr as a dependency and avoiding context (while still composing multiple css-modules), and standardizing a theme's API #33

Closed MarcusHurney closed 6 years ago

MarcusHurney commented 6 years ago

Hello Dominik and Ben,

I've been studying this library for the past couple days and a few things have crossed my mind that might make good improvements. If I'm not mistaken, the goal of react-polymorph is to provide a library of the fundamental components React developers reach for time and time again when building web apps, and also offer composable skins and themes that can be plugged into these core components. This gives developers a way to share themes and skins for these super common components in a standardized way. The goal here is to create a "standardized API" of sorts for how a theme should be constructed for a checkbox (for example). A checkbox theme should always have the css structure/classes: root, check, checked, disabled, and label. This gives the community a common point of reference for generating truly reusable and composable themes. Right now UI libraries are hard to tweak because they are abstracted away without this point of reference / common structure, and even if the styles are manipulatable, it's hard to use 3rd party UI libraries with your own custom styles without feeling like you are tearing apart the whole library or overusing "!important" for every css definition in an attempt to force your own styling protocol into the 3rd party library. Also, developers should be able to write css and scss in its native syntax instead of relying on an abstracted JS in CSS syntax that may take the language too far from its roots. This is where react-polymorph enters and tries to solve this issue that everyone has run into!

So here are the points that I've focused on in relation to react-polymorph and I would like to (hopefully) offer up a way to improve the library by reducing complexity, the chance of future issues, and also the size/dependencies of the library in its current state.

It seems that the React docs voice a general discouragement for manipulating context in React apps. --> https://reactjs.org/docs/context.html#why-not-to-use-context

With higher order components, this is sometimes unavoidable, but recently there has been a solid movement towards the "render props" pattern as a way to avoid the manipulation of context, while still retaining all the benefits/functionality of HOC's. I also think this pattern is much easier to read and you don't have to "guess" what is happening in the background with context. --> https://cdb.reacttraining.com/use-a-render-prop-50de598f11ce

There is also a similar pattern of injecting a component as a prop and rendering it that achieves the same functionality. react-css-themr relies on context and HOC's to compose multiple css modules. I think it would be a benefit to react-polymorph to remove react-css-themr as a dependency to make the library lighter weight and more flexible. The React docs also discourage inheritance when writing React Components (besides the base "extends" method for creating a class based component), so taking a more functional approach might reduce complexity. I've attached a demo repo at the bottom to make more sense of what I'm saying here. --> https://reactjs.org/docs/composition-vs-inheritance.html#so-what-about-inheritance

After looking at the source code of react-css-themr to better understand how it composes css modules, I have a working example of how this is possible without using context and without depending on react-css-themr as a 3rd party dependency while still maintaing react-polymorph's Component, Skin, Theme pattern.

The desired functionality of react-polymorph, as I understand it, is to ship components with a default theme. A component's default theme could possibly be set as a prop, but right now the default theme is more or less set in the background using the "simple" theme that Dominik has included in the library.

The desired functionality: if a user doesn't pass a component their own custom theme, the component should fall back to the default theme. If a user wants to change a few rules in the default theme but keep everything else, the user should not have to supply an entire theme of their own, but simply add their custom css definitions to the "theme" prop and have the remaining default rules for that component automatically compose with their own custom rules.

Also, when it's time to write the docs, each component should have a standardized API for how to write a custom theme (as I've mentioned above). In my demo, I've used the checkbox component and abstracted this composition process into a more modularized pattern to enforce a theme's API/structure in the file checkboxThemeAPI.js. The checkbox's theme api is also a default prop on the base Checkbox.js component. This way there is no need to use inheritance. The checkbox component is quite literally "composed" of many different parts including the skin, custom theme, default theme, and the checkbox theme api.

If you want to see the full code of the example in action, please clone here: https://github.com/MarcusHurney/polymorph-suggestions

^ I figured this demo repository would be easier than creating a pull request so we can discuss the details of what I'm suggesting more throughly.

DominikGuzei commented 6 years ago

Hey, Marcus! Thanks for your efforts, this sounds great and exactly what i wanted to actually do but didn't have the time to (we have mostly copied over the current theming pattern from react-toolbox because that's what we used before πŸ˜‰ ). The limitations of the current solution are evident and i also want to get rid of the raw folder, since it's just confusing.

So i took a look at your repository and i really like most of what i see - the only downside is that you have to pass the custom theme to any component now … and if you want to use a different default theme, you have to pass that as well. But i see how this makes it more flexible at the same time as you can choose it for each component separately.

What im missing in your example is a bit more complex component like Select (nesting multiple components), that has more logic and (in our case) is extending FormField … I would be glad to get rid of inheritance as it's more confusing than helping in many cases … in our case we saved a lot of duplication though, so we need a good replacement here!

Do you think you can continue on your prototype and implement the Select (FormField+ Bubble) in the render prop style?

MarcusHurney commented 6 years ago

Hi Dominik,

I've added the FormField and Input example to give you an idea of how render prop would look for nested components without using inheritance. I've been away for Christmas, but I'm back in San Francisco now, so I would like to keep working on this! I'm going to add the Select component with FormField next. I'll also take a look at the Bubble component as well. Any feedback on the most recent updates I've made to my demo repo are helpful! I think I've made improvements since the first checkbox example to make everything even more modular. I'm still working out refs for the input component, but I'll be finishing that up before moving on so that all the standard features of an input tag are functional (onFocus, onBlur, etc). Look forward to continuing with this!

Pull the latest on the master branch from https://github.com/MarcusHurney/polymorph-suggestions

DominikGuzei commented 6 years ago

Hey @MarcusHurney, sounds great - the latest additions are mostly repetition of the first example (from what i see) but keep going to work out the details πŸ‘ would be great if you find good solutions to these problems!

MarcusHurney commented 6 years ago

You mentioned you would like to see an example of nested components that extend FormField, which I added in the latest additions. I'm thinking maybe you didn't scroll down past the checkboxes? While running my prototype there was really no indication of any content beneath the checkboxes so I wouldn't be surprised if you just saw the original checkboxes and didn't go down further. Sorry about that!

screen shot 2017-12-27 at 8 18 50 pm

Maybe you did see those examples, but either way, I'm going to keep running with this.

DominikGuzei commented 6 years ago

Ah yeah, sorry i didn't see those examples πŸ˜‰ Ok i see, i guess it's a bit more ceremony but in the end it's cleaner & more explicit than what we have atm πŸ‘ what are the remaining challenges you have to solve?

MarcusHurney commented 6 years ago

Remaining challenges:

  1. Create a ThemeProvider component that allows a user to "inject" a theme as a prop in certain sections of their code almost like a global variable but only accessible in the section(s) of their choosing.

  2. Finish converting the remaining components into this composable pattern. What's left: Autocomplete, Button, Modal, NumericInput, Options, Select, TeaxtArea, Tooltip. Really, this isn't as daunting as it sounds because it's more a matter of restructuring parts of the components without altering their existing functionality or skins & themes very much at all, as you can see from the FormField and Input example.

  3. Figure out how to handle default themes. Right now, the only theme we have is the Simple theme, so it's serving as the default automatically.

` <Checkbox {...props} defaultTheme={'Simple'} />

or

<Checkbox {...props} defaultTheme={'Dark'} />

or

<Checkbox {...props} defaultTheme={'Light'} theme={custom.label} /> `

^ In this case, the defaultTheme and theme props will still compose together if the user passes a custom theme for a specific portion of the component's markup. Would it be better to name "theme" something more specific like "customTheme" or "styles" to make sure the differentiation between the default theme and the custom styles is more apparent?

DominikGuzei commented 6 years ago
  1. Yeah, we definitely need a propagating theme provider … the biggest concern i have with the props rendering vs. context theming (react-css-themr) is that in your current solution you have to pass the customized theme to each and every component (a lot of boilerplate). Any ideas how you would do this without React context?

  2. I agree that this won't be too much work as soon as you got the patterns down.

  3. I think for now it's fine if we just use the simple theme as default if nothing is specified. We can improve that part later on. More important in that regard is a simple way to specify another default theme for all components! Another thing im missing is the possibility to change theme variables without providing "custom theme styles" to the components. We use this a lot in our projects to customize the "simple" theme where necessary.

Regarding the component api i would rename defaultTheme to theme and the current theme to e.g: themeOverrides (not 100% sure about that one)

MarcusHurney commented 6 years ago

I believe I've made some progress on these concerns. Instead of importing parts of the Simple theme into each component in React Polymorph and setting it to the prop called defaultTheme, I created the ThemeProvider component which makes a theme object available to a specific section of code that is nested within it. In order to make this possible, I added an index.js file at -- themes/simple/index.js -- The idea here is to export the Simple theme as a JS object (works because of CSS modules). Each property on this theme object corresponds with a component offered in the React Polymorph library. This way, a theme can be passed along in its entirety (like Simple theme which covers every component in the library), or the theme object can be destructured so that the user can pass along specific properties of the theme he/she will use in the section of code nested within the ThemeProvider.

Additionally on each component, I renamed defaultTheme to "theme" and added "themeOverrides" (which can easily be renamed later).

In my prototype, I moved all code unrelated to to React Polymorph into a folder called demo (this is where I'm running the examples). It's much easier to navigate now and I think you will get the gist of everything I'm trying to describe here once you take a look at the code haha. Look at the render method of 'src/demo/app.js' to see how I'm using ThemeProvider.

Update: I've finished the ThemeProvider so that it is no longer necessary to pass both the "theme" prop and "themeOverrides" prop to every component nested within the ThemeProvider, the user only needs to pass those two props one time to the ThemeProvider. It will do the composition of the supplied base "theme" and the user's custom css from "themeOverrides" and then reveals both the base theme and a composedTheme to all nested elements. This makes it possible to use both a custom theme and the base theme within the nested code. (One Input component could use the simple theme's base input styles while another Input component within the same ThemeProvider uses a custom, composed version.

A defaultTheme could also be revealed by ThemeProvider if we want to ship the library with Simple theme built in as the default. ThemeProvider uses a new constant that I've called rootThemeAPI, which specifies the shape of a theme. This constant rootThemeAPI is defined in src/themes/API/index.js which is an object where each property is named after a type of component in the library (like Button, TextArea, Input, FormField etc). This makes it possible for ThemeProvider to compose a theme for any combination components in React Polymorph the user is nesting within ThemeProvider. The user only needs to pass the base theme and custom theme one time to ThemeProvider (which both must follow the rootThemeAPI shape). It's pretty straightforward, but seems more complicated when I try to explain it in plain English than it really is. All of these updates are on the master branch of my repo: https://github.com/MarcusHurney/polymorph-suggestions

I don't mean to be rude by doing all this in my own repo, but I figured it better to keep all this experimentation separate from the library itself. This way, we can have cleaner, clearly documented pull requests once the time comes to merge piece by piece. Getting pretty close I think!

DominikGuzei commented 6 years ago

Hey @MarcusHurney, thanks for the update - this looks really great now πŸ‘ i love that you moved the default / custom theme merging responsibility away from the individual components and into the theme provider, makes much more sense to provide a single theme property to components (they shouldn't be concerned with overrides).

Don't worry about "making your own repository", it's all open source and if you want you can run with your own ideas and create your own empire πŸ˜† we are not writing react-polymorph to become famous in the community but to solve our real-world problems … and you just helped us ;)

I think your prototype is good enough now for porting it over to our repository and refactoring the existing code base. @bendyorke please also take a look at these suggestions, i think @MarcusHurney 's changes also solve the problem you are working on regarding composing css in nested components!

bendyorke commented 6 years ago

I'm a bit late to the party here, and I'm still looking over the code, however I have some initial thoughts.

  1. I definitely like the idea of removing react-css-themr, especially if we can get rid of the raw components by doing so. I do think @DominikGuzei is right - by manually building the css-modules object, this could potentially solve a problem I've been having styling children components.

  2. While I'm personally a huge fan of render props, and understand why react warns against using context, I think HOCs and context both have their place. One place I think HOCs and context shine is in the provider pattern, like with your ThemeProvider. If ThemeProvider were a HOC, it could be declared once, and all the children could inherit from it without it needing to be re-defined.

  3. If we do decide to use the Provider pattern, then it might be worth exploring having it provide the default skins to components, as well as the themes :)

MarcusHurney commented 6 years ago

@DominikGuzei @bendyorke

Hi Ben, glad you could make it to the party! I figure now is a good time to give a brief personal introduction so that I'm less of a random guy working on this for mysterious and undisclosed reasons, haha. When I found this repo, I was doing some research on how to create and share React components in such a way that they could be super flexible and modular in functionality, markup, and style. I have many components that I would like to pluck out of various projects I've worked on over the past few years, but as it stands, this is not as easy as it sounds despite one of the core value props of React being reusability and modularity. It seems the way most people share React components is by either hosting them within in a full project on a repo or as an NPM package (which usually ships as a whole library and doesn't offer much composability or entry points for custom tweaks). For instance, trying to tweak the css of a material UI react component is a nightmare despite a lot of the components in that library being very valuable. In fact, it's almost impossible to tweak the underlying CSS in a material UI component without throwing your hands up and declaring the whole thing witchcraft after you look at the clock and realize you just spent 2 hours unsuccessfully guessing which CSS rule is overwriting your own despite adding a million !important flags.

Besides the material UI example, I'm sure both of you have come across a really interesting component, but quickly realized separating it from the context of the project in which it was created is a major headache. So instead of cloning an entire repo to extract one slice of goodness or dissecting a massive node_modules folder to simply change a few css rules, there must surely be a better way to share React components (and maybe even containers with components) in such a way that they are easily customizable and reusable in your own project. Isn't that the point of components, more or less?

When I saw Dominik's repo here, the idea of separating Components, Skins, and Themes into distinct layers that create the overall component really resonated with me. Having this distinction makes them MUCH more composable and shareable both in logic and style. I could see this pattern moving past the basic building blocks like Input, Select, Textarea etc into more use case specific instances while still maintaining the same division of concerns.

Imagine you are building an online store with React and you search for a shopping cart component. How awesome would it be to find this shopping cart component with multiple skins, themes, and composable logic (composable logic being the ability to plug different methods into the shopping cart component depending on what you need it to do). All three "layers" of this shopping cart are easily customizable and the end result can quickly be inserted into your own project. If there was an entire library of these composable components with skins and themes, I could see developers being able to browse through a huge, open source bazar of React components and pluck only what's necessary for their use cases, or contributing their own work that follows the same composable pattern for others to mix and match.

I think the direction we are heading with this library is inline with these ideas I've described, so that's one reason why I'd like to help make this a reality. Secondly, I am very interested in the Daedalus wallet and the Cardano project as a whole. The mix of helping to build this library that could really benefit the open source community at large AND contribute to one of (if not the most) interesting projects in the world is a undeniable source of motivation for me to contribute.

With that said, I also have a colleague and friend that's a developer / designer interested in this project named Dana Li. I told her that we need another theme for the library and she'd like to add a theme. As for me, I'm going to port the rest of the components into the new pattern we've been discussing here. I'm planning to make the pull requests organized by each component, so there is a bit of structure and reference points to the changes being made. I'd also like to get with Ben on how to improve ThemeProvider after I get these core changes out of the way. Thanks guys!

hellodanali commented 6 years ago

Hi guys,

Marcus showed me some of the stuff he's been working on here, and he mentioned adding another theme. I put together a mockup based on some of the colors from the Daedalus landing page. I'm planning to start coding this theme out sometime this week. If you have any suggestions on this theme so it serves as a balanced alternative to simple theme, I'd like to hear your feedback.

screen shot 2018-01-03 at 4 36 39 pm

MarcusHurney commented 6 years ago

Just a heads up, so you guys and gals aren't in the dark with what I've been doing. I'm still refactoring over the remaining components and skins to match our latest upgrades. Today I finished, TextArea, Button, Tooltip, and Bubble (Input, FormField, and Checkbox are already finished). Additionally I added an autoFocus prop and functionality to the input and textarea components, and also worked out the tricky nature of passing ref's from a parent component down into a child component. It's surprisingly awkward how it works, but it gets the job done and I don't see it getting any prettier in that respect! I'd like to finish this refactor before the end of the week so I can start organizing the pull requests needed to get this live. I'm still maintaining my live prototype of these changes so everyone can test and play around with them before pulling. Hope all is well, looking forward to hear from guys! As for Dana, I work closely with her on a day to day basis and can assist her navigation of the library any further. We spent a good bit of time digging into this today and I think she is excited to contribute.

DominikGuzei commented 6 years ago

Hey peoplez, welcome to the party @hellodanali! Thanks for all the great input πŸ‘

@MarcusHurney i will review your first PR today but i like the general direction you outlined here!

@hellodanali thanks for your first theme draft :1st_place_medal: my feedback:

@hellodanali do you work with Sketch? it would be awesome if we could establish a components theme file for a widespread application like sketch which can be adjusted by any theme designer πŸ˜ƒ we could easily create a simple plugin script that exports the colors to CSS variables

hellodanali commented 6 years ago

Hi @DominikGuzei,

Awesome to hear from you! I've made some changes and dialed down the contrast between the green and the background color. I did make these in Sketch. Were you thinking of turning these components or the ones from simple theme into Sketch symbols in a re-usable Sketch file for developing component themes?

screen shot 2018-01-05 at 1 00 00 pm

DominikGuzei commented 6 years ago

Hey @hellodanali, That looks much better πŸ‘ yeah it would be awesome if we could have a re-usable and easily adjustable Sketch theme file, that can be used to create new versions. Later on we can write a Sketch plugin that exports the variables so that it can be directly used in code (or at least with little effort)

hellodanali commented 6 years ago

@DominikGuzei

Ok, sounds good. I'm go ahead and start working out the Sketch theme file on these.

MarcusHurney commented 6 years ago

@DominikGuzei @bendyorke

Hey guys, I refactored ThemeProvider according to Ben's suggestions. I definitely agree that passing a theme object to child components via context is cleaner than trying to accurately follow its trickle downward from a high level and potentially through many nested components. The ThemeProvider accepts theme and themeOverrides as props, like before. What's nice is that the user can pass an entire theme object to the ThemeProvider that covers every component in the library, or just the keys of a theme that match the components he/she is using as children of ThemeProvider in that specific instance. If themeOverrides is also passed, it will be composed with the theme prop and then made available via context as theme. So the user only receives context.theme in their components regardless of whether it's composed or not. Alternatively, the user can forgo passing themeOverrides and just pass it to a single instance of a child component and the component itself will compose themeOverrides with its copy of the theme object in context. This seems pretty flexible and relatively lightweight. I added some performance considerations to ThemeProvider so it only re-renders if its props have changed.

I'm also going to refactor the stories to use ThemeProvider which really cuts back on some of the noise. Other than that, I am still working on refactoring the last few components to use the render prop architecture.

Here is a look at ThemeProvider: https://gist.github.com/MarcusHurney/bbb7aa1e568920d68f931fca31f64b9b

Thanks for the suggestion there Ben!

hellodanali commented 6 years ago

@DominikGuzei

I organized the sketch file so that all the components can be styled by changing shared styles in the Styleguide folders. Let me know if this is set up the way you have in mind. Currently, the components do not save into the particular classNames designated for each component. Is there a particular naming convention I need to follow for the sketch files?

daedalus - compactTheme.zip

MarcusHurney commented 6 years ago

@DominikGuzei @nikolaglumac

Hi guys,

I bet you are probably pretty busy with the paper wallet certificate generator, which is undoubtedly more exciting than sweating over specificity in CSS πŸ˜‰ , but I've continued researching other projects that are tackling the same or similar issues we're facing with react-polymorph and I've found an example I think we should look into as a showcase for how this library could successfully serve its intended purpose at scale. Dominik mentioned that it's important for the future apps on Daedalus to have a cohesive style so it doesn't appear Frankenstein. I think it's very tricky to enforce a style guide on third party developers without being super rigid and making people resent the development process, πŸ‘Ž. Building an app is already hard enough, and needing to use third party components is probably a big turnoff unless they are highly intuitive, flexible, and general when they need to be.

Although the existing components offered here cover most of the basics, it's inevitable that other developers will need to write a significant amount of external html and css to "publish" a robust app on Daedalus. With that in mind, I foresee an added layer of difficulty for them to match the styles of their own components with the themes that exist here. Today, I read this blog post from the engineering team at Yelp that discusses a similar challenge they faced when trying to enforce their style huide across multiple apps with many developers and how they solved it using React and CSS Modules. It's actually not that long of a read, but I already feel like "that guy" for asking you to read this comment and that article 😞 .

I think what's missing from react-polymorph are the general components because what we have now are pretty intuitive and flexible. If you look at Yelp's style guide, you'll see they have components that address more general styling concerns like containers, layout utilities, and typography. They haven't open sourced the 65 component library mentioned in the blog post, but the style guide pretty much covers every "general" use case I can think of at the moment. I also think their lemon-reset library mentioned in the blog post is interesting beyond the fact it resets global styles and more because it creates super generalized/low level components that are more like intuitive building blocks.

Main Point: Enforcing a style guide seems more reasonable and painless for third party developers by offering a combination of low level and high level components.

DominikGuzei commented 6 years ago

Thanks for the great input @MarcusHurney. You're right, we definitely need an easy and flexible way to build composed UIs, that can be themed together. Especially with our new research pointing more and more in the direction of using JSX over JSON for 3rd party applications. Instead of giving them any kind of way to directly manipulate the DOM / CSS, they will have to communicate their UI in a special/limited JSON format, that will be parsed and rendered by the main render process.

@hellodanali can you take a look at the current Daedalus design specs and work out a basic (very basic) style guide for typography and layout containers that we could provide?

DominikGuzei commented 6 years ago

@hellodanali sorry for the (very) late answer: yeah that's the right direction … although i think Sketch will be too limited to generate a complete configuration. I guess we need to provide a theme builder UI as a website at some point, something like jQuery UI themeroller

DominikGuzei commented 6 years ago

closing this because we have resolved everything that was discussed in the latest versions πŸ‘

HiirenP commented 5 years ago

@DominikGuzei @MarcusHurney @bendyorke @hellodanali @clemenshelm Hello All, I used react-css-themr to implement delicious theme into my component(e.g. button) so for component project it works fine (like priority is ---> external theme 1st and if not getting external theme(button.scss from delicious theme folder) it use internal theme (button.scss from component's scss folder)).

this is working fine for this project. but if i deploy this project and use as library into other project (like button component to my main project)then it always loaded default css instead of external css. can any one please help me in this?

thanks!!!!

DominikGuzei commented 5 years ago

Hello @HiirenP, thanks for your question!

We moved away from using react-css-themr a while ago and introduced our own ThemeProvider!

You can use it like this: (using latest release version 0.8.0):

import React from "react";
import { Input } from "react-polymorph/lib/components";
import { InputSkin } from "react-polymorph/lib/skins/simple/InputSkin";
import { SimpleTheme } from "react-polymorph/lib/themes/simple";

const MyForm = () => (
  <div>
    <Input label="First Name" skin={InputSkin} />
    <Input label="Last Name" skin={InputSkin} />
  </div>
);

const SimpleFormApp = () => (
  <ThemeProvider theme={SimpleTheme}>
    <MyForm />
  </ThemeProvider>
);

My latest PR on develop branch added the feature to also provide the skins via the provider like this:

import React from "react";
import { Input } from "react-polymorph/lib/components";
import { SimpleSkins } from "react-polymorph/lib/skins/simple";
import { SimpleTheme } from "react-polymorph/lib/themes/simple";

// Notice that we don't have to pass any skin or theme to the inputs:
const MyForm = () => (
  <div>
    <Input label="First Name" />
    <Input label="Last Name" />
  </div>
);

const SimpleFormApp = () => (
  <ThemeProvider skins={SimpleSkins} theme={SimpleTheme}>
    <MyForm />
  </ThemeProvider>
);

Can you give that a try? If you still have issues please send some code examples or a repo where i can see what's going on πŸ˜‰

HiirenP commented 5 years ago

@DominikGuzei Thanks!! for the help i will surely give this a try in demo, but in my regular project i cant because i have reached at peak and from here i cant afford to make global changes. 😊 Thanks again!!