Skip to content
This repository has been archived by the owner on Jan 13, 2023. It is now read-only.

Allow style arrays in HOCs (WIP) #86

Merged
merged 24 commits into from
Oct 5, 2018
Merged

Allow style arrays in HOCs (WIP) #86

merged 24 commits into from
Oct 5, 2018

Conversation

morgandonze
Copy link
Contributor

@morgandonze morgandonze commented Sep 7, 2018

See #79

I have enabled style arrays in HOC components, and I have provided Storybook examples for each.
The example styles are not meant to look good.

@derekgreenberg the "trick" I mentioned was this:

  if (Array.isArray(props.fillStyle)) {
    fillStyle = reduce((acc,term) => {
      return { ...acc, ...term }
    }, FILL, props.fillStyle)
  } else {
    fillStyle = { ...FILL, ...props.fillStyle } as ViewStyle
  }

to properly reduce the array style. I tried something before this that didn't work (something like the following):

ramda.mergeWith(ramda.concat, FILL, styleOverride)

(Not really a trick as much as simply doing it right instead of wrong)

screen shot 2018-09-27 at 3 05 19 pm

screen shot 2018-09-27 at 3 05 24 pm

screen shot 2018-09-27 at 3 05 31 pm

screen shot 2018-09-27 at 3 05 37 pm

screen shot 2018-09-27 at 3 05 39 pm

screen shot 2018-09-27 at 3 05 47 pm

screen shot 2018-09-27 at 3 05 56 pm

screen shot 2018-09-27 at 3 05 59 pm

@morgandonze morgandonze changed the title Allow style arrays in HOCs Allow style arrays in HOCs (WIP) Sep 7, 2018
@@ -17,7 +18,7 @@ const OUTLINE: ViewStyle = {
marginTop: 2, // finicky and will depend on font/line-height/baseline/weather
justifyContent: "center",
alignItems: "center",
borderWidth: 1,
borderWidth: 1,//
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove // here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For each of the component changes, you should provide a storybook example.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mlaco pull from master, which has changes to a bunch of storybook stories. Then update storybook stories for each component you have modified here.

Copy link
Contributor

@skellock skellock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you change our definitions from say ViewStyle to StyleProp<ViewStyle>, you might not need much of this code. I always just use ViewStyle instead of ViewStyle[] since it's easy to spread & overwrite things. I can't tell you when the last time I used an array for styles was.

React Native supports passing an {}, [], true, false, and StyleSheet.

And you can mix & match any of the above into an []. And that's what StyleProp does for us.

The only thing would need to change is how the style merging inside the components that add their own styling (like CheckBox). But you could use an array now since you can't spread it (since it's not a single object-based type).

Life is simpler with spreads, imo.

@morgandonze
Copy link
Contributor Author

Hey @kevinvangelder, as you can see, I implemented this. But Steve brought up concerns
about adding unnecessary code, and I agree with it. Array styles don't appear
to provide any meaningful benefit.

I can’t think of a real reason someone would need an array style, and if they had some weird situation where they dynamically compute something that comes out as an array, they could just reduce it themselves.

Unfortunately I didn't consider the necessity before writing this PR, but I'm not sure merging it is the best idea. What are your thoughts?

@jamonholmgren
Copy link
Member

I have seen array styles taught a lot in React Native courses. So, even though we don't necessarily use them in-house, it might make sense to still have examples of how to handle them here. Just my 2 cents.

@kevinvangelder
Copy link
Member

I use arrays for styles almost everywhere. If I'm understanding Steve correctly we could still support array styles by just changing our types and making minor changes to the implementation? @skellock could you give us an example?

@morgandonze
Copy link
Contributor Author

In honor of @skellock, I will commence with the merging of code he doesn't like 😉

@skellock
Copy link
Contributor

skellock commented Oct 5, 2018

Haha ❤️ All good. I may not be a fan but if many people are, then give 'er.

@morgandonze morgandonze merged commit 1b45dbc into master Oct 5, 2018
@jamonholmgren jamonholmgren deleted the style-arrays branch October 16, 2018 21:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants