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

Improve/add handling of default values #1136

Open
alexandernanberg opened this issue Feb 13, 2020 · 11 comments
Open

Improve/add handling of default values #1136

alexandernanberg opened this issue Feb 13, 2020 · 11 comments

Comments

@alexandernanberg
Copy link

Is your feature request related to a problem? Please describe.

When building UI libraries you want to set good defaults but let consumers override if necessary. Which doesn't really work with the padding/margin currently. So there is no clear way of defining default values that works in all situations.

Eg here https://codesandbox.io/s/pedantic-davinci-r78wx?fontsize=14&hidenavigation=1&theme=dark, as a consumer of the Box component I expect that I'll be able to overwrite the left padding.

Describe the solution you'd like

A better way to provide default values, using defaultProps isn't always enough. Currently I've created a wrapper around system, I wish something similar was available natively. The caveat with this solution is that it doesn't work with compose which is kind of a deal breaker.

export function system(config) {
  const parser = systemCore(config);
  parser.withDefaults = defaults => props => parser({ ...defaults, ...props });

  return parser;
}

Describe alternatives you've considered

N/A

Additional context

N/A

@The-Code-Monkey
Copy link

Isnt this PR what you are trying to accomplish #886

@alexandernanberg
Copy link
Author

@The-Code-Monkey I don't think so, my issue has nothing to do with the theme, only the default values

@The-Code-Monkey
Copy link

The-Code-Monkey commented Feb 18, 2020

Ok looking at your example it seems that the defaultProps styles are getting applied to the component after your props styles are.

Could you not use themeGet() rather than using the defaultProps so your component would look like this

const Box = styled.div`
  padding: ${themeGet("spacing.4")}
  background-color: red;
  ${compose(padding)}
`;

that would get you the same thing only different is you would need a themeProvider rather than relying on the inbuilt ones.

@alexandernanberg
Copy link
Author

I could, but it's not as convenient imo. We have a few more advanced styled-system fns that returns more than one css rule, for those cases it doesn't work great.

Having to wrap everything in a ThemeProvider isn't always a viable solution either.

Another downside of your proposed solution is that the default css rule always will be rendered (it won't merge the rules). So with the padding: ${themeGet('spacing.4')}; rule and I pass <Box p={7} /> it will get both padding: 12px; padding: 40px;

@bernatfortet
Copy link

@alexandernanberg did you end up figuring this one out? I'm running into the same problem.

@alexandernanberg
Copy link
Author

@bernatfortet I ended up forking styled-system and added withDefaults on the parser, I can create a gist of the entire source if you'd like

@The-Code-Monkey
Copy link

A gist would be nice or even put up a pr for your issue

@alexandernanberg
Copy link
Author

Here is the gist https://gist.github.com/alexandernanberg/e00bdb42fd44ad14c9d1d86127d696a3

It also includes a fix for undefined values which I have a PR up for here #1394

@bernatfortet
Copy link

Thank you so much for the gist.
I'm very curious about what you did, could you help me understand what withParser does?
Also, did you try to run the benchmarks?
My main goal with updating from v4 to v5 is to improve rendering performance.
Thanks!

@alexandernanberg
Copy link
Author

I don't remember exactly what changes I made to make it work, but you can probably find out by diffing the gist against https://github.com/styled-system/styled-system/blob/master/packages/core/src/index.js

The use case would looks something like

const styleProps = compose(layout, space).withDefaults({
  display: 'flex',
  p: 4,
});

which would set display: flex and padding: 16px unless you pass props to it

Haven't done any benchmarks, but I suspect that it performs slightly worse than v5 because we add the withDefaults fn to each parser

@bernatfortet
Copy link

Gotcha. That's what I thought.

I actually found another way. It reuses the mapProps function from v4 and adds it to space.js
The code is slightly different from v4 because this way makes sure only defined props get passed.
I did run the benchmarks and there was no alteration to v5 scores.

// fork or recreate space.js
export const is = n => n !== undefined && n !== null

export const mapProps = mapper => func => {
  const next = props => func(mapper(props))
  for (const key in func) {
    next[key] = func[key]
  }
  return next
}

export const space = mapProps(props => {
  const p = { ...props }

  if (props.my) p.mt = props.my
  if (props.my) p.mb = props.my

  if (props.mx) p.ml = props.mx
  if (props.mx) p.mr = props.mx

  if (props.py) p.pt = props.py
  if (props.py) p.pb = props.py

  if (props.px) p.pl = props.px
  if (props.px) p.pr = props.px

  return p
})(
  compose(
    margin,
    padding,
  )
)

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

No branches or pull requests

3 participants