patternfly / patternfly-react

A set of React components for the PatternFly project.
https://react-staging.patternfly.org/
MIT License
794 stars 357 forks source link

PF4: Override react-core styles #681

Closed dlabrecq closed 6 years ago

dlabrecq commented 6 years ago

I would like to override the CSS embedded in the react-core components. As an example, consider the CSS image used with the new backgroundImage component. I could change the $pf-global--image-path variable in patternfly-next and build my own patternfly.css. However, I cannot do that with react-core. In fact, the component's embedded CSS overrides my own because of the order it's included in the page.

It seems that the order in which webpack loads styles depends on the order the modules are imported. Thus, I'm having a very hard time overriding embedded component styles in the Koku UI because the react-core component CSS is always output after mine. I'm sure there are workarounds, but would be far easier if to include the react-core CSS myself, similar to patternfly.css.

I realize that we can override components individually, via the className property. However, it would be preferable if the component honored my patternfly theme.

Note that patternfly-ng stopped inlining CSS in the components for similar reasons.

dlabrecq commented 6 years ago

I was able to override background images using new variables from PR https://github.com/patternfly/patternfly-next/pull/776; however, I want to note that any CSS that does not have a variable still could not be overridden. For example, if I wanted to change width or height, I cannot easily do that.

.pf-c-background-image {
  position: fixed;
  top: 0;
  left: 0;
  z-index: -1;
  width: 100%;
  height: 100%;
  background-color: var(--pf-global--BackgroundColor--dark-200);
  background-image: var(--pf-c-background-image--BackgroundImage--xs);
  filter: var(--pf-c-background-image--Filter);
  background-repeat: no-repeat;
  background-size: cover;
dlabrecq commented 6 years ago

I have to take back what I said regarding being able to override variables -- those were global variables only. I'm not able to override local component variables.

The screenshot below shows what happens when CSS is embedded in our react-core components. Webpack loads my styles first (highlighted in yellow), while the react-core component styles are output later; thus, overriding my previously defined variables.

screen shot 2018-10-02 at 3 39 44 pm

Note that I added local variables to patternfly-next's background image for this example. However, I believe it would be the same problem for any CSS property embedded by a react-core component.

dmiller9911 commented 6 years ago

I replied in the other repo, but will provide the response here as well for others:

This is a mixture of a few things

  1. Injection order as you stated. This can be avoided by making the selector body .pf-c-background-image instead. Adding the body reference should make it more specific and force it to use it instead.
  2. Webpack css-loaders will likely still try to load all the image paths even if they are overridden by a css variable. Without seeing the css file you made it is possible that the actual path being loaded is not working either. These doc files are copied into react-docs so you will want to make sure the path is correct relative to the copied file for build time.

Note. For this repo instead of using import foo from 'bar.css' (which looks to be how it was loaded based on the style tags) use @patternfly/react-styles you can either use the injectGlobal or use StyleSheet.create. StyleSheet.create will be preferable since you can scope it to the div itself, and will be closer to actual consumption.

const styles = StyleSheet.create({
  backgroundExample: `
    & .pf-c-background-image { //all overrides in here }
 `
});

class Example extends React.Component {
  render()  {
    return (
      <div className={css(styles.backgroundExample)}>
        <BackgroundImage />
      </div>
    )
  }
}

It is even possible you might be able to use a selector like &.pf-c-background-image, and then pass the className directly to the background image component.

Then in a consuming project you would likely set that in and export the component with that className always appended to it.

import { BackgroundImage as PFBackgroundImage } from '@patternfly/react-core';  

const styles = StyleSheet.create({
  backgroundImage: `
    &.pf-c-background-image { //all overrides in here }
 `
});

export const BackgroundImage = (props) <PFBackgroundImage {...props} className={css(styles.backgroundImage)} />
dlabrecq commented 6 years ago

Thank you for the suggestions, Doug. body .pf-c-background-image seems to work, but I'm concerned with the specificity. That is, I'm wondering how difficult it would be when a developer needs to override that same react-core component style?

The StyleSheet example may be a better approach for us. I was able to override the variables react-docs (Gatsby) is using with &.pf-c-background-image; however, I also want to override the variables used by the react-core component itself.

When testing react-docs, I see the styles for both the component's overrides and the overrides for the example. While testing the component with the Koku UI, I don't see the component's overrides loaded in the page like I do when testing react-docs. Only when I add similar overrides to Koku UI directly, then I can see my styles loaded in the page.

How do we get the react-component (from the dist dir) to output its own styles? Are we doing anything special with emotion to load StyleSheet in react-docs?

Update: I was able use emotion.cx to inject the backgroundImage override styles myself, bypassing react-core's StyleSheet. (May be a bug in StyleSheet preventing those styles from being output?) Regardless, the paths do not appear to be relative to the component. Therefore, it looks like each app would need to override the paths as Doug suggested above. It's close, but having apps define the images is less than ideal.