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

Box component - what is the best way of cleaning up props rendered on the dom when using styled? #1844

Open
blurymind opened this issue Feb 23, 2021 · 11 comments
Labels

Comments

@blurymind
Copy link

blurymind commented Feb 23, 2021

Describe the bug
I have a box component made using the styled-system props. Its not done via emotion. The problem is that all box props end up on the dom. I am wondering if there is some sort of a clean solution for this that wouldnt require adding tons of extra code to maintain.

To Reproduce
Here is some code you can use to reproduce it

import styled from "styled-components";
import { color, layout, position, space, border, shadow, flexbox, compose, system, textAlign } from "styled-system";

export const BoxStyledProps = compose(space, color, layout, position, border, shadow, flexbox, textAlign);
export const StyledBox = styled.div(BoxStyledProps);

Expected behavior
None of the props I pass to my StyledBox end up on the actual dom

Screenshots
image
color and height should not be there

Additional context
I know there is some doc about this somewhere, but it really isnt clear on how to actually deal with the situation in the context of using the true power of styled-system props for creating a box

@blurymind blurymind added the bug label Feb 23, 2021
@LoiKos
Copy link

LoiKos commented Mar 8, 2021

Hello,

related docs are : https://styled-system.com/guides/removing-props-from-html

Docs exposed 2 methods to avoid passing props to HTML. In your case you need the first one using this package :
npm i @styled-system/should-forward-prop

The docs only mentioned emotion but it should work with styled-components as well. You need to use this https://styled-components.com/docs/api#shouldforwardprop

styled-system/should-forward-prop give you all props links to styled-system but actually you can also add yours if you need to add more 😄

@marcin-piechaczek
Copy link

With styled components you can do something like this:

import styled, { css } from 'styled-components';
import { color, typography, layout, border, shadow, space } from 'styled-system';
import { props } from '@styled-system/should-forward-prop';

const Typography = styled('span').withConfig({
  shouldForwardProp: (prop, defaultValidatorFn) => !props.includes(prop) && defaultValidatorFn(prop)
})`
  ${color}
  ${typography}
  ${layout}
  ${border}
  ${shadow}
  ${space}
`;

export default Typography;

I'm looking for a global place to add those settings before each styled component, here is a related issue.

@blurymind
Copy link
Author

blurymind commented Mar 12, 2021

@marcin-piechaczek thanks this is very helpful :) It does have one disadvantage and that is having to add withConfig to every component that takes in box props.

Is it possible to abstract this somehow?

Lets say I have this in one place

// Box component
export const BoxStyledProps = compose(space, color, layout, position, border, shadow, flexbox, someUtilProps, textAlign);

and then every component I want to get box props for me has this

const MyOtherComponent = styled.div`
....
  ${BoxStyledProps}
`;

const AnotherComponent = styled.div`
....
  ${BoxStyledProps}
`;

In this example MyOtherComponent and AnotherComponent take in box props, is there a way to not have to add withConfig to both of them and somehow keep it in one place - preferably the Box component?

@The-Code-Monkey
Copy link

@LoiKos The shouldForwardProp built into styled-system is for emotion only.

@blurymind
Copy link
Author

@LoiKos The shouldForwardProp built into styled-system is for emotion only.

it does appear to work in styled too, there is just the problem I mentioned in my previous post

@marcin-piechaczek
Copy link

@blurymind You can solve this problem with the @The-Code-Monkey solution mentioned here.

Use your own method to import/export styled with withConfig method.

@LoiKos
Copy link

LoiKos commented Mar 12, 2021

@The-Code-Monkey yes it is. But i believe it's because the docs have been written before styled-components provide an official way to handle that. When I was looking for withConfig juste before it was out, the docs was already the same on that parts.

@blurymind to solve your problem instead of using div you can use whatever component you want e.g:

const Wrapper = styled.div.withConfig(**your config goes here**)

const MyOtherComponent = styled(Wrapper)`
....
  ${BoxStyledProps}
`;

const AnotherComponent = styled(Wrapper)`
....
  ${BoxStyledProps}
`;

@LoiKos
Copy link

LoiKos commented Mar 12, 2021

Another solution is to keep adding withConfigto each component when needed and to build a common shouldForwardProp mecanism e.g:

// props-blocker.js
import { props } from '@styled-system/should-forward-prop'

const toRemoveProps = [
  ...props,
]  // You can add whatever prop you like here

const regex = new RegExp(`^(${toRemoveProps.join('|')})$`)

const shouldForwardProp = (prop, defaultValidatorFn) => {
  return !regex.test(prop)
}

export { shouldForwardProp }


// ComponentA.js
import {shouldForwardProp} from 'path/to/props-blocker.js'
const ComponentA = styled.div.withConfig({ shouldForwardProp })``

@blurymind
Copy link
Author

blurymind commented Mar 12, 2021

@LoiKos thanks for the suggestions

I want to avoid doing this

const Wrapper = styled.div.withConfig(**your config goes here**)

const MyOtherComponent = styled(Wrapper)`
....
  ${BoxStyledProps}
`;

specifically styling the wrapper which is in itself a styled component noticeably impacts mount time when you have lots of components rendered

On the second suggestion - do you think memoizing shouldForwardProp could be benefitial? Its kind of a pain, but I need to find the cheapest approach possible

@LoiKos
Copy link

LoiKos commented Mar 12, 2021

Sorry not sure to understand your question. You are supposed to provide a callback as shouldForwardProp but unfortunately it's up to styled-component to run it so you can't really control how it will be used.

Depend how it work internally inside Styled-component. Anyway the function you provided in production will never change so i don't think you need to memoize it.

@marcin-piechaczek
Copy link

Here is the updated info about using shouldForwardProp with styled-components:

import styled from 'styled-components';
import { space, layout } from 'styled-system';
import shouldForwardProp from '@styled-system/should-forward-prop';


const Wrapper = styled.div.withConfig({
  shouldForwardProp
})`
  ${space};
  ${layout};
  position: relative;
  height: 42px;
  border: 0;
  padding: 0;
  margin-top: 20px;
`;

There are currently no official methods to apply withConfig globally. You can achieve this by making your own "styled" wrapper, but I encountered problems in situations where the styled parameter was another component or an SVG file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants