Allow all UI components to accept extra class names and modifiers #33

wyuenho · 2016-12-19T21:08:35Z

A lot of the components have hard coded class names and states that get appended to the class names, but we can't append to the class names when customizing, either to override the styles in a specific component in a certain context, or introduce contextual BEM modifiers that will only be apparent at the use site.

jwineman · 2016-12-19T21:11:32Z

How is this different from #31?

Not exposing extra class names is by design.

wyuenho · 2016-12-19T21:14:36Z

The rationale here doesn't seem to make sense anymore. In practice, CSS styles will leaks into every component because inheritance, and there's no way to firewall that from the JS side. Designers also have a lot of requirements that we have to implement as exceptions to the rules, we need a way to override the default classes on demand.

jwineman · 2016-12-19T21:20:29Z

I think a lot of these issues stem from the fact that we haven't finished the backbone migration yet.

The component based styles don't use inheritance, the BEM styles are specific to the component and don't inherit styles from anywhere else.

I would argue we want to push back against designers making exceptions to the rules - every exception they want to create makes the code base harder to maintain.

If we make exceptions we should style them in the CSS for the page, not the component.

The main value in using a UI library is the standardization of the components, this breaks that and I think its worth looking at other solutions.

wyuenho · 2016-12-19T22:18:04Z

Typographic styles are inherited almost all the time, they affect sizes, in which there's no one-size-fits-all approach will work.

The major problem now is there's no way to pass the props down to the inner components. Using page level styles is a non-solution. Many components like modal, tooltip etc don't get attached to a specific page. The form elements in them also often have style adjustments that we have to deal with. Without being able to add specific class names and modifiers, it's very hard to impossible to target these elements.

jwineman · 2016-12-19T22:42:41Z

<Heading/> and <Text/> and cf-component-typography don't have inherited styles right?

I think <Modal/> is a special case that might warrant exception. What about exposing an CSS ID on <Modal/> instead of full on style/class props?

What is an example of a form element that can't be styled at the page level?

wyuenho · 2016-12-19T23:04:53Z

We have global typographic styles, and we'll always have them, unless you introduce a <Body /> component. IMHO, those typographic cf-ui elements are superfluous. We aren't going to use <Heading/> in every place we have to use a heading....

wyuenho · 2016-12-19T23:06:32Z

ID is also a terrible idea. Nobody is going to check if that ID exists already before putting an ID

jwineman · 2016-12-19T23:25:01Z

I think the point of using a UI library is to avoid global styles and I disagree we'll always need to have them.

If you style typography at the component level, and always use <Heading\> when you need a heading then you avoid inherited styles - which is the root problem of this github issue.

I'm not really in favor of using CSS IDs either but I guess I don't understand the issue which is why I was asking for a specific example. What is wrong with this approach for page level styling of <Modal/>?

<Modal>
          </ModalHeader>
          <ModalBody>
            <div className="edit-subscription-modal">
             ...
             </div>
          </ModalBody>
          <ModalFooter>
            <ModalActions>
              <button onClick={this.handleRequestClose.bind(this)}>Close Modal</button>
            </ModalActions>
          </ModalFooter>
        </Modal>

wyuenho · 2016-12-20T00:50:06Z

The point of having a UI library is to enable logic reuse, not avoid global styles. There will always be a couple of links that need a couple of different colors, a few buttons that have so much text, the paddings and margins of their parents have to be readjusted etc. Yes we can wrap things in <div> s, but these are unnecessary DOM elements, and the CSS the results from them tend to be rather complex. The browser already has a DOM, CSS 3 allows us to target any tree structure we want, why do we have to have React components backing the most mundane HTML elements in JS? They don't do anything, all they do is increase memory consumption and slow down rendering.

Also, text styles like font-size will almost always be inherited unless you explicitly override them in every single element style, which is an immensely terrible idea. CSS inherits from the DOM structure, not what you put in your React component. The advantage of having global basic styles is you don't have to define it everywhere, but you can if you want to. The problem with cf-ui now is exactly the opposite. Instead of using native HTML elements as a baseline, we are using an extra layer of JS components as a baseline, which forces us to wrap extraneous HTML elements to customize, and potentially lose the encapsulated logic, just because we want it to look slightly different. Normally, we can just extend the component, keep the logic and rewrite the markup, but hey, that's not allowed too... The Liskov substitution principle baby gets thrown out with the bathwater.

The advantage of allowing extra class names is it enables the generated markup to be contextual and semantic, while giving us an anchor point for CSS selectors to target a whole section of decedents without introducing extra <div>s everywhere like the early 2000. The advantage of allowing arbitrary BEM modifiers to be passed in is, as opposed to what the discussion document says, it keeps the customization concern to the library users, without affecting the logic in the components themselves. The reason for allowing both is, BEM modifiers should only convey generic types and states, but they shouldn't provide any targeted context like a specific area. You may very well change a CSS property in a class name and effect unintended consequences outside of what you are trying to fix.

toekneestuck · 2016-12-20T04:21:51Z

I still want to see an example of where this is necessary/desired. Let's be honest, we haven't made a ton of progress on the migration yet. I'm not convinced on the amount of pain this will eliminate in the short term versus create in the long. We've seen what mistakes can be made by allowing too much arbitrary flexibility in components (aka, everything I wrote 3+ years ago).

wyuenho · 2016-12-20T17:15:40Z

This is a larger issue we'll encounter whether or not we have the entire app written in React. The Backbone stuff we have now is actually a combination of trying to generalize everything with configs, misunderstanding of the relationship between Backbone's model/collection/view, and how to arrange the views. Our customized ItemView is a major culprit, since it breaks the contract of both ItemView and CollectionView, spreading everything across dozens of JS module repos is another. Some modules are too tightly defined, like the modal. We have a number of modals that skip/have irregular steps, share data, and have different button combination and behavior. They are very hard to create using our internal backbone modal component.

Here's how I imagine it should be done.

Forms. We only allow 2 layouts now, and it's required. We simply cannot use them for what we are working on, because we have an irregular form that is required but does not fit into the 2 layouts. This is even more restrictive than Bootstrap. These are purely stylistic props, I don't think any modifier should be required, and if they are, the enumeration should be extensible without having the library user to come back to add one whenever there's something they need.

For modals, we often have buttons with really long text in the footer, page rules and dedicated certs come to mind. What we often have to do is to override the button alignments and shrink the text. Some cards like Edge Certs also have really long buttons, and the parent containers, after being wrapped into a couple of divs, we no longer have control of the paddings and margins. There are other places where we have to override the bottom margin of some list or heading because they are too tall. We can either allow the ability to append arbitrary class names to override containers, or do this:

<ModalFooter>
  <div className="some-specific-footer">
    <Button>{{"text"}}</Button>
    <Button> <!-- How do I align this, another span wrapping it again? -->
      <Text size="small">
        {{"some really really long text that should be in a span or some phrasing content, but now it's in a div, and after we get rid of the block, the sizes still aren't right because the ideal size should be between small and normal so now we have to wrap yet another span but div under button still violates HTML spec oh god why"}}
      </Text>
    </Button>
  </div>
</ModalFooter>

/* yuck */
.some-specific-footer {
  text-align: left;
  .cf__button:nth-child(2) {
    .cf__text--small {
       display: inline;
       span {
         font-size: 14px;
       }
    }
  }
}

Or more simply, just give us back semantic markup, let us append a class name to the container that our custom component renders without having to target decedents in a maze of CSS. So we can do this instead:

<ModalFooter classNames={{['some-specific-footer']}}">
  <Button>{{"text"}}</Button>
  <Button classNames={{['special-button']}}>
      {{"some really really long text"}}
  </Button>
</ModalFooter>

/* Much cleaner */
.some-specific-footer {
  text-align: left;

  .special-button {
    padding: 10px;
    font-size: 14px;
  }
}

Granted, this is just 1 specific instance where a combination of existing cf-component deficiency manifest. We can make <Text> to render a <span> when we ask it to, and just use react-dom's form, but we can do better then this. We don't need the typographical components, and we don't need required purely stylistic props. Just let a global base style to take care of styling basic things like text elements and links, and give us the ability to override when we need to. Essentially, just let us use react-dom.

jamiebuilds · 2017-01-13T21:04:45Z

I don't know why this issue is being made to be so complicated when it's not

Classes should be directly tied to the components which they are applied to. Scoping should be eliminated as much as possible, if not altogether. The reasons for this are based on the ideas behind using inline styles: http://blog.vjeux.com/2014/javascript/react-css-in-js-nationjs.html

The only reason inline styles are not being used on cf-ui is because we were not allowed to open source CloudFlare's styles due to phishing concerns.

When this was going on, I spent some time designing an API which would allow us to specify inline styles separately. The reason I did not implement it at the time because I was busy trying to figure out how to migrate the mess of styles written in SASS. I can share this API separately if anyone is interested.

You should be treating the public API of cf-ui components as if they are using inline styles. Sure there are some limitations to inline styles, but working through that was an intentional pain point because it prevents you from hacking something together which will only add to code debt.

Use composition to create UIs instead of one-off-ing everything.
Use propTypes to create well designed APIs for components.
Work together with designers to ensure consistent UIs and to reuse UI elements.

Let yourselves feel a little bit of pain so that you can create a better end result. Do not make the same mistakes that ~~that dumbass~~ @toekneestuck made

wyuenho · 2017-01-14T01:38:05Z

I'm just going to paste this link here for the many counter points against inline CSS.

https://css-tricks.com/the-debate-around-do-we-even-need-css-anymore/

My favorite is, I don't want to see !important everywhere we need to override inline styles.

In practice, we have not seen one single React UI component framework that doesn't allow externally supplied className, I take that as a signal that no one else found this approach practical. Also, composition isn't always possible. HTML is very specific about what elements are allowed where, we can't just wrap a <div> around a <tr> and expect it to work everywhere. In addition, you can have inline children under blocks, but not the other way around.

There's a reason I haven't implemented this tho, I'm still debating whether we should go all out BEM, and reset every HTML element in every cf-component wrapper. There's some investment in tooling we have to do, but the benefit is we can still avoid inline styles like the plaque, (but can if we want to, supplied from both externally and internally), and have consistent, predictable style without having to worry about specificity and inheritance. The downside is obviously the component specific reset will still reset every wrapper override, and thus make inline styles a necessity.

I'm still tending toward keeping global baseline styles as that CSS Tricks article suggest at the bottom.

jamiebuilds · 2017-01-14T21:50:49Z

we have not seen one single React UI component framework that doesn't allow externally supplied className

That's because most UI libraries aren't built by the sole company who uses them. cf-ui isn't designed to be general purpose, CF can make any change to it at any time. This concern isn't valid.

I take that as a signal that no one else found this approach practical.

I've spoken to the React team directly and the teams that have been building React components for years at Facebook about this specific thing – it is considered by many to be a good practice.

Inline styles is considered a best practice by pretty much the entire React community. You are going against the grain of thousands of developers who have more experience than this team.

wyuenho · 2017-01-14T22:53:10Z

How do they deal with pseudo classes and media queries?

jamiebuilds · 2017-01-15T04:44:47Z

There's a slightly out of date comparison of a lot of the libraries here: https://github.com/FormidableLabs/radium/blob/master/docs/comparison/README.md

In that list these are all the libraries that support pseudo classes and media queries: Radium, React Inline, React JSS, ReactShadow, React Free Style, React Inline CSS, React Look, React Statics Styles, react-styl, smart-css, Stilr

However, there's three that are not on that list that I currently find the most interesting:

https://github.com/zeit/styled-jsx (half runtime, half babel build step)
https://github.com/threepointone/glamor (implements a number of ideas from libraries that came before it)
https://github.com/rtsao/styletron (Blog: https://ryantsao.com/blog/virtual-css-with-styletron)

^ All three of those support pseudo elements and media queries as well

The cool thing about inline styles is that when you decide to use them it's really easy to do major refactors across all of your styles. This is why you see people iterating so quickly on different ideas for libraries.

We're starting to get into the phase where people are building inline style tools that can optimize your css just by nature of how they work (ex. virtual css w/ styletron). The benefits to inline CSS are only going to keep growing over time.

wyuenho · 2017-01-15T18:09:19Z

Haha I knew you were going to say something along that line.

To use media queries, besides extracting out an actual .css file, I've seen people embedding <style> elements into JSX or intermixing it with the document, or use window.matchMedia(). A problem with the last 2 approaches is they are both blocking operations. Browsers, for a very long time, have been optimized for constructing the CSSOM first before rendering happens. Not doing so will result in a lot of sudden flashes of style application (repaint/layout) as the page is rendered. That's why all web performance guidelines suggest we put the stylesheets in the head of the document. window.matchMedia is even worse as it goes into JS land, so repaint and layout get delayed even further. Pseudo classes is the same in that they have to be emulated via events processed in JS. Unfortunately, that's Radium.

I don't see Radium and all the other ones in that comparison will catch on until the browsers do something about these problems (i.e. allow media queries and pseudo class in inline styles, and provide a JS API at the HTMLElement level for them, or better, Shadow DOM). Either that or provide an easy way for us to extract a global style sheet out of the JS code base (Radium knock knock). Styletron is something I've never heard of before and it looks interesting. It claims it has a solution for the critical rendering path problems but it doesn't say how it works. It appears to have hooks to extract a global style sheet from JS. I'll look into this one.

A lot of the recent trends in React come from people who want to bring mobile development approaches to the Web without understanding the limitations. Mixing structure/look and feel/behavior into the same files is something the Web community hasn't explored in over a decade, I think it's wise to take a cautious approach instead of jumping into the new and shiny right away.

P.S. This post is a bit outdated, but I think it's still worth a read.

wyuenho · 2017-01-15T20:29:50Z

@thejameskyle Just so we are clear, do you suggest us wrapping every cf-ui component with our internal component in order to apply some styles?

wyuenho · 2017-01-15T20:52:30Z

https://github.com/MicheleBertoli/css-in-js

This list seems to be more up-to-date. The ideal feature set should be 1,1,1,0,1 (need real CSS for fallbacks) or 0,1,1,0,1 (assuming autoprefixing can be added to the CSS extracted). There are only 2 tools that satisfy 1,1,1,0,1 (cssx, styled-jsx), 7 for 0,1,1,0,1

jamiebuilds · 2017-01-16T00:46:45Z

Haha I knew you were going to say something along that line.

@wyuenho you have a serious attitude problem.

I've gone out of my way to help CF's open source in my free time. You are representing a company on a company project, you need to act like a professional.

wyuenho added the enhancement label Dec 19, 2016

wyuenho mentioned this issue Dec 21, 2016

Optional custom class names for table components #43

Merged

wyuenho mentioned this issue Jan 4, 2017

Format classnames with classnames #49

Closed

jamiebuilds closed this as completed Jan 16, 2017

jamiebuilds reopened this Jan 16, 2017

tajo closed this as completed Mar 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow all UI components to accept extra class names and modifiers #33

Allow all UI components to accept extra class names and modifiers #33

wyuenho commented Dec 19, 2016

jwineman commented Dec 19, 2016

wyuenho commented Dec 19, 2016

jwineman commented Dec 19, 2016 •

edited

wyuenho commented Dec 19, 2016

jwineman commented Dec 19, 2016

wyuenho commented Dec 19, 2016

wyuenho commented Dec 19, 2016

jwineman commented Dec 19, 2016 •

edited

wyuenho commented Dec 20, 2016 •

edited

toekneestuck commented Dec 20, 2016

wyuenho commented Dec 20, 2016 •

edited

jamiebuilds commented Jan 13, 2017 •

edited

wyuenho commented Jan 14, 2017 •

edited

jamiebuilds commented Jan 14, 2017

wyuenho commented Jan 14, 2017

jamiebuilds commented Jan 15, 2017 •

edited

wyuenho commented Jan 15, 2017 •

edited

wyuenho commented Jan 15, 2017

wyuenho commented Jan 15, 2017

jamiebuilds commented Jan 16, 2017 •

edited

Allow all UI components to accept extra class names and modifiers #33

Allow all UI components to accept extra class names and modifiers #33

Comments

wyuenho commented Dec 19, 2016

jwineman commented Dec 19, 2016

wyuenho commented Dec 19, 2016

jwineman commented Dec 19, 2016 • edited

wyuenho commented Dec 19, 2016

jwineman commented Dec 19, 2016

wyuenho commented Dec 19, 2016

wyuenho commented Dec 19, 2016

jwineman commented Dec 19, 2016 • edited

wyuenho commented Dec 20, 2016 • edited

toekneestuck commented Dec 20, 2016

wyuenho commented Dec 20, 2016 • edited

jamiebuilds commented Jan 13, 2017 • edited

wyuenho commented Jan 14, 2017 • edited

jamiebuilds commented Jan 14, 2017

wyuenho commented Jan 14, 2017

jamiebuilds commented Jan 15, 2017 • edited

wyuenho commented Jan 15, 2017 • edited

wyuenho commented Jan 15, 2017

wyuenho commented Jan 15, 2017

jamiebuilds commented Jan 16, 2017 • edited

jwineman commented Dec 19, 2016 •

edited

jwineman commented Dec 19, 2016 •

edited

wyuenho commented Dec 20, 2016 •

edited

wyuenho commented Dec 20, 2016 •

edited

jamiebuilds commented Jan 13, 2017 •

edited

wyuenho commented Jan 14, 2017 •

edited

jamiebuilds commented Jan 15, 2017 •

edited

wyuenho commented Jan 15, 2017 •

edited

jamiebuilds commented Jan 16, 2017 •

edited