nachoaIvarez / flexbox-react

Unopinionated, standard compliant flexbox component. No propietary APIs. Nothing but flexbox.
Other
319 stars 49 forks source link

4.3.0 will force children to be recreated - does not happen in 4.2.1 #26

Closed xaviergonz closed 7 years ago

xaviergonz commented 7 years ago

Today I updated to 4.3.0 and suddenly I noticed that children component inside the flexbox were being completely recreated (instead of updated) upon new renders.

When I went back to 4.2.1 the issue was fixed.

PS: not sure if it helps, but the root of the cause seems to be re-creations of styled.div children I think.

xaviergonz commented 7 years ago

I just noticed that you are basically creating a styled(element) on each render. Maybe that's why? Would it be better to make FlexBox a full class component and only recreate (and cache in a class property) this styled component on componentWillMount + componentWillUpdate?

xaviergonz commented 7 years ago

Ok, basically that worked, but I still had to use the props inside the styled element itself for it to work.

Here is the code I did (and tested) in typescript (tsx file). This way it doesn't recreate the children as it used to in 4.2.1

Sorry I don't have time for a proper pull request, but it should be fairly easy to translate back to jsx if you so wish.

import * as React from 'react';
import {PropTypes} from 'react';
import styled from 'styled-components';
import CSSWideKeyword = React.CSSWideKeyword;

const { string, oneOf, oneOfType, node, number, object } = PropTypes;

const toPropertyString = (name: string, value: any): string => {
  // hyphenate the camel case name
  const realName = name.replace(/([A-Z])/g, (g: any) => `-${g[0].toLowerCase()}`);

  return value ? `${realName}: ${value};` : '';
};

const CSSWideKeywordStrings = [ 'initial', 'inherit', 'unset' ];

export type FlexboxElement = 'article' | 'aside' | 'div' | 'figure' | 'footer' | 'header' | 'main' | 'nav' | 'section';

const possibleElements: FlexboxElement[] = [
  'article',
  'aside',
  'div',
  'figure',
  'footer',
  'header',
  'main',
  'nav',
  'section',
];

export interface FlexboxProps {
  children?: React.ReactNode;
  className?: string;
  element?: FlexboxElement;
  style?: React.CSSProperties;

  alignContent?: CSSWideKeyword | 'flex-start' | 'flex-end' | 'center' | 'space-between' | 'space-around' | 'stretch';
  alignItems?: CSSWideKeyword | 'flex-start' | 'flex-end' | 'center' | 'baseline' | 'stretch';
  alignSelf?: CSSWideKeyword | 'auto' | 'flex-start' | 'flex-end' | 'center' | 'baseline' | 'stretch';
  display?: 'flex' | 'inline-flex';
  flex?: CSSWideKeyword | string;
  flexBasis?: CSSWideKeyword | string;
  flexDirection?: CSSWideKeyword | 'row' | 'row-reverse' | 'column' | 'column-reverse';
  flexFlow?: CSSWideKeyword | string;
  flexGrow?: CSSWideKeyword | number;
  flexShrink?: CSSWideKeyword | number;
  flexWrap?: CSSWideKeyword | 'nowrap' | 'wrap' | 'wrap-reverse';
  justifyContent?: CSSWideKeyword | 'flex-start' | 'flex-end' | 'center' | 'space-between' | 'space-around' | 'space-evenly';
  order?: CSSWideKeyword | number;

  height?: CSSWideKeyword | string;
  margin?: CSSWideKeyword | string;
  marginBottom?: CSSWideKeyword | string;
  marginLeft?: CSSWideKeyword | string;
  marginRight?: CSSWideKeyword | string;
  marginTop?: CSSWideKeyword | string;
  maxHeight?: CSSWideKeyword | string;
  maxWidth?: CSSWideKeyword | string;
  minHeight?: CSSWideKeyword | string;
  minWidth?: CSSWideKeyword | string;
  padding?: CSSWideKeyword | string;
  paddingBottom?: CSSWideKeyword | string;
  paddingLeft?: CSSWideKeyword | string;
  paddingRight?: CSSWideKeyword | string;
  paddingTop?: CSSWideKeyword | string;
  width?: CSSWideKeyword | string;
}

class Flexbox extends React.Component<FlexboxProps, {}> {
  static defaultProps: Partial<FlexboxProps> = {
    display: 'flex',
    element: 'div',
  };

  static propTypes: any = {
    children: node,
    className: string,
    element: oneOf(possibleElements).isRequired,
    style: object,

    alignContent: oneOf([
      'center',
      'flex-end',
      'flex-start',
      'space-around',
      'space-between',
      'stretch',
      ...CSSWideKeywordStrings
    ]),
    alignItems: oneOf(['baseline', 'center', 'flex-end', 'flex-start', 'stretch', ...CSSWideKeywordStrings]),
    alignSelf: oneOf(['baseline', 'center', 'flex-end', 'flex-start', 'stretch', ...CSSWideKeywordStrings]),
    display: oneOf(['flex', 'inline-flex']).isRequired,
    flex: oneOfType([string, oneOf(CSSWideKeywordStrings)]),
    flexBasis: oneOfType([string, oneOf(CSSWideKeywordStrings)]),
    flexDirection: oneOf(['column-reverse', 'column', 'row-reverse', 'row', ...CSSWideKeywordStrings]),
    flexGrow: oneOfType([number, oneOf(CSSWideKeywordStrings)]),
    flexShrink: oneOfType([number, oneOf(CSSWideKeywordStrings)]),
    flexWrap: oneOf(['nowrap', 'wrap-reverse', 'wrap', ...CSSWideKeywordStrings]),
    justifyContent: oneOf(['center', 'flex-end', 'flex-start', 'space-around', 'space-between', ...CSSWideKeywordStrings]),
    order: oneOfType([number, oneOf(CSSWideKeywordStrings)]),

    height: string,
    margin: string,
    marginBottom: string,
    marginLeft: string,
    marginRight: string,
    marginTop: string,
    maxHeight: string,
    maxWidth: string,
    minHeight: string,
    minWidth: string,
    padding: string,
    paddingBottom: string,
    paddingLeft: string,
    paddingRight: string,
    paddingTop: string,
    width: string,
  };

  private static _styledComponents?: any = undefined;

  componentWillMount(): void {
    if (Flexbox._styledComponents) return;

    // initialize all possible _styledComponents

    // take the prop names from the propTypes but for some that don't need to be styled
    const propNames: string[] =
      Object.keys(Flexbox.propTypes).filter((e: string) =>
      e !== 'children' && e !== 'element' && e !== 'style' && e !== 'className');

    Flexbox._styledComponents = {};
    possibleElements.forEach((e: string): void => {
      Flexbox._styledComponents[e] = styled(e as any)`
        ${(p: FlexboxProps): string => propNames.map((name: string): string => toPropertyString(name, p[name])).join('')}
      `;
    });
  }

  render(): JSX.Element {
    const {element, children, ...otherProps} = this.props;
    const StyledComponent = Flexbox._styledComponents[element!.trim().toLowerCase()];
    return (<StyledComponent {...otherProps}>{children}</StyledComponent>);
  }
}

export default Flexbox;
xaviergonz commented 7 years ago

PS: I'm not sure if styled components do anything with the style attribute. Probably it would need to be removed.

On the other hand, if it does work maybe className would need to be added as well.

It would need to be added to propTypes and ( e !== 'children' && e !== 'element' && e !== 'style' && e !== 'className')

xaviergonz commented 7 years ago

Ok, apparently both style and className are supported, so I just added them to the source code above. Also I added some proper typescript typing, so it can be easily translated into a definitions file by the typescript compiler.

Cheers!

jeffbonasso commented 7 years ago

I will add to this...since upgrading to 4.3 I started seeing strange issues. If wherever I have a Flexbox component I replace with <div style={{display: flex etc...) with the same properties as passing to Flexbox then everything is fine. I love the approach with this component where it insulates the developer from dealing with all the intricacies of all the proper Flexbox styles across browsers as well as the simple React tag, properties for everything. Right now I went back to 4.2.1 and all is good. Great job with the component!

hampusohlsson commented 7 years ago

@xaviergonz @jeffbonasso I can shed some light to why v4.3.0 uses the styled(element). It's because the previous approach with inline styles doesn't work with server side rendering, but using styled-components fixes that issue. I'd be happy to assist in fixing any issue you guys are seeing with this approach.

bjjensonop commented 7 years ago

I just upgraded and am seeing the same issue. styled-components creates a new class upon each DOM update which reloads all child components.

hoppula commented 7 years ago

Having the same problem, version 4.3.0 broke our app. When parent elements are recreated all the child inputs lose focus etc.

nachoaIvarez commented 7 years ago

If you could submit a PR with that fix it would be awesome @hampusohlsson! 😄

nachoaIvarez commented 7 years ago

Or you @xaviergonz, it would be highly appreciated! 😃

hoppula commented 7 years ago

@nachoaIvarez I believe @xaviergonz is right, the problem is that you're creating the styled component inside pure component's render (https://github.com/nachoaIvarez/flexbox-react/blob/master/src/Flexbox.jsx#L41), you should move StyledComponent outside the Flexbox function and just prefix everything with props., like align-content: ${props => props.alignContent ? props.alignContent : ''};. Didn't test it yet though :)

hoppula commented 7 years ago

I just noticed the dynamic tag name (element), that makes moving it outside more complicated.

hoppula commented 7 years ago

Check this implementation and test case: https://www.webpackbin.com/bins/-Kfrfs0WOsUM4RIc3m_j

nachoaIvarez commented 7 years ago

v4.3.1 is out! Thanks @hoppula for your help!

hampusohlsson commented 7 years ago

Great job guys, thanks @hoppula !!