Accordion - Give ViewProps Priority

[jessedoyle]

Hi There! Thanks for making a great package!

React Native Collapsible really simplifies our code and is great to work with!

One issue we encountered is that we needed to pass down a style prop to the Accordion container View here. Unfortunately, the Collapsible component defines a style prop here.

The code was written to give the Collapsible component's props priority when building out the proxy prop objects. This PR switches the logic such that React Native's View component props are first resolved, then the Collapsible props are resolved.

This allows the caller to pass down a style prop to the root Accordion view.

[iRoachie]

Instead of changing the logic would it not be easier to add a style prop on the Accordion?

[jessedoyle]

Instead of changing the logic would it not be easier to add a style prop on the Accordion?

That would definitely work and was my first thought as well.

Ultimately the reason I went with swapping the logic was that an explicit case would have to be added here for the style prop if we were to add any direct props.

i.e.

Object.keys(this.props).forEach(key => {
  // style is a valid prop for Collapsible, but we want to accept it
  // as viewProps, not collapsibleProps...
  if (COLLAPSIBLE_PROPS.includes(key) && key !== 'style') {
    collapsibleProps[key] = this.props[key];
  } else if (VIEW_PROPS.includes(key)) {
    viewProps[key] = this.props[key];
  }
});

The solution above seemed a little kludgy, so I opted to swap the logic instead.

@iRoachie - Do you have any thoughts/suggestions?

[iRoachie]

That's not how you would do it, the forEach statement works by checking ViewPropTypes, which should already have style in it.

So this should actually work if you just place <Accordion style={mystyle}

[jessedoyle]

@iRoachie

That's not how you would do it, the forEach statement works by checking ViewPropTypes, which should already have style in it.

Definitely, but the Collapsible also has a style prop defined: https://github.com/oblador/react-native-collapsible/blob/master/Collapsible.js#L16.

The forEach statement checks the COLLAPSIBLE_PROPS first, then checks the VIEW_PROPS second.

Unless I'm missing something, the current logic will always apply a style prop to collapsibleProps, not viewProps as intended.

[jessedoyle]

So this should actually work if you just place <Accordion style={mystyle}

I've tried this, but it unfortunately doesn't work as the styles are applied to the Collapsible component here.

The more I think about it, it seems like a new prop should be introduced on Accordion to handle container styling. Maybe containerStyles?

There may be people relying on the current (broken) behaviour.

[jessedoyle]

@iRoachie - I updated the commit to use a different approach.

Instead, a containerStyle prop may now be passed to style the root View component.

[iRoachie]

Great i much more prefer this. Can you also add this property to the typescript definitions?

[jessedoyle]

@iRoachie - I've added the typescript definition!

[iRoachie]

THanks! 💯