Skip to content
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

Ability to set default shouldForwardProp or expose to extended components #1394

Open
rathpc opened this issue Jun 10, 2019 · 6 comments
Open

Ability to set default shouldForwardProp or expose to extended components #1394

rathpc opened this issue Jun 10, 2019 · 6 comments
Milestone

Comments

@rathpc
Copy link

@rathpc rathpc commented Jun 10, 2019

UPDATED TO REFLECT MORE ACCURATE ISSUE

The problem

I would love the ability to set a default shouldForwardProp value, project wide by either a simplistic factory extension or via a theme. Currently when I set the shouldForwardProp value on a styled div it works great. However if I extend that styled component to form another one leveraging the withComponent helper, that shouldForwardProp value is not exposed to that new styled component and I need to set it again.

ie:

import { css } from '@emotion/core';
import styled from '@emotion/styled';
import * as styledSystem from 'styled-system';
import { COLORS, fontAR, shouldForwardProp } from '../../constants/styles';

const baseStyle = css`
  ${fontAR};
  font-size: 14px;
  line-height: 1.43;
`;

const smallStyle = css`
  font-size: 12px;
  line-height: 1.67;
`;

const largeStyle = css`
  font-size: 16px;
  line-height: 1.91;
`;

const boldStyle = css`
  font-family: Avenir;
  font-weight: 900;
`;

const styleMap = {
  small: smallStyle,
  large: largeStyle,
  bold: boldStyle,
};

interface ParagraphProps
  extends styledSystem.ColorProps,
    styledSystem.FontFamilyProps,
    styledSystem.FontSizeProps,
    styledSystem.FontWeightProps,
    styledSystem.HeightProps,
    styledSystem.LetterSpacingProps,
    styledSystem.LineHeightProps,
    styledSystem.SpaceProps,
    styledSystem.TextAlignProps,
    styledSystem.WidthProps {
  color?: COLORS;
  size?: string;
}

export const Paragraph = styled('p', { shouldForwardProp })<ParagraphProps>`
  ${baseStyle}
  ${styledSystem.color}
  ${styledSystem.fontFamily}
  ${styledSystem.fontSize};
  ${styledSystem.fontWeight}
  ${styledSystem.height}
  ${styledSystem.letterSpacing}
  ${styledSystem.lineHeight}
  ${styledSystem.space}
  ${styledSystem.textAlign}
  ${styledSystem.width}
  ${props => (props.size ? styleMap[props.size] : null)}
`;

const Label = styled(Paragraph)`
  color: ${COLORS.slate};
  display: flex;
  align-items: center;
  margin-bottom: 4px;
`.withComponent('label');

Now Label does not know to exclude any props that are a result of the imported function which works as intended in the Paragraph component.

Proposed solution

  • Add the ability in the core code to consume the underlying shouldForwardProp value if a styled component is extending another styled component combined with withComponent.

It works if you use the as prop but my intention is to not need to remember to add that additional prop anytime I want to use the new Label component

@FezVrasta

This comment has been minimized.

Copy link
Collaborator

@FezVrasta FezVrasta commented Jun 10, 2019

I wonder if CacheProvider could help here

@Andarist

This comment has been minimized.

Copy link
Member

@Andarist Andarist commented Jun 10, 2019

However if I extend that styled component to form another one, that shouldForwardProp value is not exposed to that new styled component and I need to set it again.

Created component should "inherit" shouldForwardProp:

shouldForwardProp =

If it's not the case please provide a codesandbox with the issue reproduced.

@rathpc

This comment has been minimized.

Copy link
Author

@rathpc rathpc commented Jun 11, 2019

This is totally my mistake, I lost track of where the issue was actually happening. It loses "inheritance" when combined with withComponent

I will update the original code example above but also putting here for reference

import { css } from '@emotion/core';
import styled from '@emotion/styled';
import * as styledSystem from 'styled-system';
import { COLORS, fontAR, shouldForwardProp } from '../../constants/styles';

const baseStyle = css`
  ${fontAR};
  font-size: 14px;
  line-height: 1.43;
`;

const smallStyle = css`
  font-size: 12px;
  line-height: 1.67;
`;

const largeStyle = css`
  font-size: 16px;
  line-height: 1.91;
`;

const boldStyle = css`
  font-family: Avenir;
  font-weight: 900;
`;

const styleMap = {
  small: smallStyle,
  large: largeStyle,
  bold: boldStyle,
};

interface ParagraphProps
  extends styledSystem.ColorProps,
    styledSystem.FontFamilyProps,
    styledSystem.FontSizeProps,
    styledSystem.FontWeightProps,
    styledSystem.HeightProps,
    styledSystem.LetterSpacingProps,
    styledSystem.LineHeightProps,
    styledSystem.SpaceProps,
    styledSystem.TextAlignProps,
    styledSystem.WidthProps {
  color?: COLORS;
  size?: string;
}

export const Paragraph = styled('p', { shouldForwardProp })<ParagraphProps>`
  ${baseStyle}
  ${styledSystem.color}
  ${styledSystem.fontFamily}
  ${styledSystem.fontSize};
  ${styledSystem.fontWeight}
  ${styledSystem.height}
  ${styledSystem.letterSpacing}
  ${styledSystem.lineHeight}
  ${styledSystem.space}
  ${styledSystem.textAlign}
  ${styledSystem.width}
  ${props => (props.size ? styleMap[props.size] : null)}
`;

const Label = styled(Paragraph)`
  color: ${COLORS.slate};
  display: flex;
  align-items: center;
  margin-bottom: 4px;
`.withComponent('label');

Now Label does not know to exclude any props that are a result of the imported function which works as intended in the Paragraph component.

@Andarist

This comment has been minimized.

Copy link
Member

@Andarist Andarist commented Jun 11, 2019

Could you prepare a codesandbox with the issue reproduced? Copy-pasted code is still a far way from getting this running and debuggable and I, unfortunately, don't have time to do this right now.

@rathpc

This comment has been minimized.

Copy link
Author

@rathpc rathpc commented Jun 17, 2019

Could you prepare a codesandbox with the issue reproduced? Copy-pasted code is still a far way from getting this running and debuggable and I, unfortunately, don't have time to do this right now.

Here is a sandbox showing the issue:

https://codesandbox.io/s/emotion-issue-template-uz95h?fontsize=14

Example 1 should be working just like the other two but does not currently. The ideal situation is for it to work just like Example 2 which I found as a workaround for now. Example 3 is the fail-proof solution but requires more code, more often.

Example 4 is an extra situation I decided to illustrate that shows while the workaround Example 2 does work, it only works one level deep since color re-appears in the DOM render in this example.

Let me know if you need any other clarification. Thanks 👋

@Andarist

This comment has been minimized.

Copy link
Member

@Andarist Andarist commented Jun 17, 2019

Ok, I see what's happening now. When using withComponent this line "fails" and things are not being merged like you might have expected.

Need to think about how this and other options are handled when extending components

@FezVrasta FezVrasta removed the needs triage label Oct 25, 2019
@Andarist Andarist added this to the v11 milestone Nov 3, 2019
animecyc added a commit to animecyc/emotion that referenced this issue Dec 3, 2019
- Fixes a bug that prevented `shouldForwardProp` from forwarding to components created via `withComponent`; Fixes emotion-js#1394
- Compacts `createStyled`'s options argument
animecyc added a commit to animecyc/emotion that referenced this issue Dec 5, 2019
- Fixes a bug that prevented `shouldForwardProp` from forwarding to components created via `withComponent`; Fixes emotion-js#1394
- Compacts `createStyled`'s options argument
animecyc added a commit to animecyc/emotion that referenced this issue Dec 5, 2019
- Fixes a bug that prevented `shouldForwardProp` from forwarding to components created via `withComponent`; Fixes emotion-js#1394
- Compacts `createStyled`'s options argument
animecyc added a commit to animecyc/emotion that referenced this issue Dec 5, 2019
- Fixes a bug that prevented `shouldForwardProp` from forwarding to components created via `withComponent`; Fixes emotion-js#1394
- Compacts `createStyled`'s options argument
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.