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

Simplify the utils API #186

Closed
braposo opened this issue Sep 5, 2020 · 5 comments · Fixed by #209
Closed

Simplify the utils API #186

braposo opened this issue Sep 5, 2020 · 5 comments · Fixed by #209

Comments

@braposo
Copy link

braposo commented Sep 5, 2020

Is your feature request related to a problem? Please describe.
I've been using stitches in a side-project and one of the things that I noticed is that there could be some room for simplifying the utils API.

Currently we need to use nested functions to define a new util:

utils: {
    m: (config) => (value) => ({
      marginTop: value,
      marginBottom: value,
      marginLeft: value,
      marginRight: value,
    })
}

But in most cases we don't really use the config value so having to always declare that is just creating noise in the config. Granted that it isn't much and we only have to define it once, but reducing the surface of code is generally a good thing so I thought about opening this for discussion.

On that point, it also feels that for the majority of the cases the utils are used to compute or derive values from the initial input, so maybe we could go even further with that simplification.

Describe the solution you'd like
For the first case could we simply reduce it to a single function and have the config value being sent as an optional second parameter of the function?

utils: {
    m: (value: string, config?: Config) => ({
      marginTop: value,
      marginBottom: value,
      marginLeft: value,
      marginRight: value,
    })
}

On the second case, what do you think of supporting a syntax like:

utils: {
    m: ['marginTop', 'marginBottom', 'marginLeft', 'marginRight'],
    size: ['height', 'width'],
	
	linearGradient: (value) => ({
      backgroundImage: `linear-gradient(${value})`,
    })
}

If you don't think this mixed syntax makes sense to have in utils, maybe consider having a computed or shortcuts entry and leave utils for the more complex cases like the linear gradient?

Thanks for this amazing library, looking forward to seeing it grow!

@braposo
Copy link
Author

braposo commented Sep 5, 2020

Just to add on that second suggestions, this is how the official utils example would look like with that syntax:

export const { styled, css } = createStyled({
  utils: {
    m: ['marginTop', 'marginBottom', 'marginLeft', 'marginRight'],
    mt: ['marginTop'],
    mr: ['marginRight'],
    mb: ['marginBottom'],
    ml: ['marginLeft'],
    mx: ['marginLeft', 'marginRight'],
    my: ['marginTop', 'marginBottom'],
    size: ['width', 'height'],
	br: ['border-radius'],

    linearGradient: (value) => ({
      backgroundImage: `linear-gradient(${value})`,
    }),
  },
});

I think this reduces the "boilerplate" considerably and make it a bit more readable as well.

@christianalfoni
Copy link
Contributor

Ah, yes, the reason it worked like this is because with the functional API that stitches originally had you could pass multiple arguments to a utility. So you could say like:

{
  someUtil: (config) => (arg1, arg2, arg3) => ....
}

But since there can only be a single argument to a utility now it makes more sense to put the config as a second argument for sure.

{
  someUtil:  (arg1, config) => ....
}

@peduarte
Copy link
Contributor

peduarte commented Sep 7, 2020

I totally agree that's a decent API simplification. All in for this moving the config as a second argument there.

@braposo regarding the second suggestion, I like it the idea, but imo it makes things more complex and increases the API surface

I think having one way to do something, even though it's a bit more typing, gives users a consistent DX.

Thanks for the suggestion though, appreciate that.


Action for this issue

So for this issue, let's address removing the nested function. Move the config argument as a second argument.

@braposo
Copy link
Author

braposo commented Sep 7, 2020

Thanks for the explanation @christianalfoni, that makes total sense! I also wondered if you were imagining people would be using function composability to avoid some duplication like

{
  m: (config) => marginAll(config)
}

but that isn't probably a common use case and can still be done with the combined args anyway.

Which actually leads me to the second suggestion, and first of all thanks for giving your reasoning as well @peduarte.

I completely agree with you and when I made the proposal were kind of thinking in the setState API from React where you have a "quick" way to set the state setState({ key: value }) and a more "advanced" way (which actually ends up being the most common one) like setState(() => ({ key: value })).

But I still agree with what you say and think that probably at this stage of the maturity of the library it doesn't make sense to include these syntax variations.

I was thinking that this could be done on user-land instead and was going to ask you if we could build a stitches utils instead but just found out that there's actually https://github.com/hauptrolle/stitches-utils#readme from @hauptrolle which is exactly what I was referring to in my comment.

@peduarte
Copy link
Contributor

peduarte commented Sep 8, 2020

@braposo exactly 💯

We have plans to make these reusable utils. And a whole community driven set of "add ons"

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

Successfully merging a pull request may close this issue.

3 participants