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

[FR]: Default style property in useSx option #39

Closed
1 task done
mym0404 opened this issue Mar 20, 2024 · 13 comments · Fixed by #56
Closed
1 task done

[FR]: Default style property in useSx option #39

mym0404 opened this issue Mar 20, 2024 · 13 comments · Fixed by #56
Assignees
Labels
enhancement New feature or request released

Comments

@mym0404
Copy link
Member

mym0404 commented Mar 20, 2024

Describe the problem

At now, the getStyle function takes SxProps as a parameter for passing additional style properties.

It is handled with lowest priority because it is used for default property sometimes.

We should separate a concern between defining default style properties and passing additional style properties.

Describe the solution

Step 1. Create default(not fixed name) options to useSx hook.

const { /* ... */ } = useSx(props, {
  default: {
    borderWidth: 1,
  },
});

Important

This SxProps should be handled as most lowest priority

Step 2(NEED DISCUSSION). The priority of SxProps as getStyle parameter should be highest one?(lower than style).

I have no idea how this priority should be updated.

Note

We should update this new behaviors in the docs.

Code of Conduct

  • I agree to follow this project's Code of Conduct
@mym0404 mym0404 added the enhancement New feature or request label Mar 20, 2024
@mym0404
Copy link
Member Author

mym0404 commented Mar 20, 2024

@zmin9 need discussion

@zmin9
Copy link
Collaborator

zmin9 commented Mar 20, 2024

I think SxProps passed to getStyle should have the highest priority.
Given the usage(passing style prop to component → useSx → getStyle), it makes sense that they would be applied last.

@mym0404
Copy link
Member Author

mym0404 commented Mar 20, 2024

I think SxProps passed to getStyle should have the highest priority. Given the usage(passing style prop to component → useSx → getStyle), it makes sense that they would be applied last.

I think the priority of the useSx-default properties should be lower than passing style prop to component. Because if the priority of default is higher than it, there is no way to overwrite default properties declared in component.

@zmin9
Copy link
Collaborator

zmin9 commented Mar 20, 2024

I think SxProps passed to getStyle should have the highest priority. Given the usage(passing style prop to component → useSx → getStyle), it makes sense that they would be applied last.

I think the priority of the useSx-default properties should be lower than passing style prop to component. Because if the priority of default is higher than it, there is no way to overwrite default properties declared in component.

oh I agree on lowest priority of useSx-default prop.
It is shortcut to set default style without thinking about the priority between style, sx, SxProps of component props. So convenient!

// without default props
useSx({
  style: {
     borderWidth: 1, // it overwrite component sxProps although it is default style
     ...props?.style,
  },
  /* ... */
   ...props,
})

To be clear, it would be good that the default of useSx had the lowest priority and SxProps of getStyle had the highest priority.

@mym0404 mym0404 pinned this issue Mar 20, 2024
@mym0404
Copy link
Member Author

mym0404 commented Mar 20, 2024

Right, let's do like this.

@zmin9 zmin9 self-assigned this Mar 20, 2024
@mym0404
Copy link
Member Author

mym0404 commented Mar 20, 2024

@zmin9 Can you name default properties as fallback? I think it is more meaningful than default.

Also, please review this whether does my think seem to correct.

@zmin9
Copy link
Collaborator

zmin9 commented Mar 21, 2024

@mym0404
While resolving this issue, I'd like to discuss: do we really need sxProps for getStyle?
I don't think we will use it often, and if we do, it could be written like this:

<View style={[getStyle(), { backgroundColor: 'red }]} />

Also, I think the priority with transform is confusing.

@mym0404
Copy link
Member Author

mym0404 commented Mar 21, 2024

I agree with that. Did you read the updated docs for this?

@mym0404
Copy link
Member Author

mym0404 commented Mar 21, 2024

If passing plain old style into style will be a alternative or recommended way of this package, then the main benefit of this package(easing of styling in various theme) would be broken(even if we can't prevent use like that by useSxTokens).

I am curious about sx parameter of getStyle is really required in any aspect of usages.

Conversely, finding any use-cases that can't be covered with fallback or transform would help us for the decision.

@zmin9
Copy link
Collaborator

zmin9 commented Mar 22, 2024

If we need to use the state value of some child component, as in the example below, getStyle-sxProps would be needed 😅 . Though this case is not common, it is difficult to be covered with fallback or transform.

Or why not just have useSx return the getThemedStyle(sx?:SxProps) function?
I think it would be confusing to use getStyle(sx?: SxProps) with priority.

<View style={[getThemedStyle({mt: 2}), getStyle(), getThemedStyle({mb: 4})]} />

@zmin9
Copy link
Collaborator

zmin9 commented Mar 22, 2024

+)
And then, I read this docs, do you think this getStyle-sxProps should have higher priority than transform?
I naturally assumed that the getStyle would be affected by transform because it is the return value of useSx with transform property.

@mym0404
Copy link
Member Author

mym0404 commented Mar 22, 2024

From this discussion, it was more clear that getStyle-sx is so confusing.

I think we have to remove it.(with introducing fallback).

In your reply, getThemedStyle is a good catch.

How about this?

Create a new hook useSxStyle

const getS = useSxStyle();

return <View style={getS({ mt: 2, /* ... */ })}/>

I insist that useSx shouldn't have two or more usage other than props -> style.
If we create a new hook for the pure nothing -> style usage, useSx hook is separated from the other usage.

Note

I created useSxStyle and merged it.

@mym0404 mym0404 mentioned this issue Mar 22, 2024
2 tasks
mym0404 added a commit that referenced this issue Mar 25, 2024
Resolve #39 Default style property in `useSx` option
@mym0404
Copy link
Member Author

mym0404 commented Mar 25, 2024

🎉 This issue has been resolved in version 3.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants