search

nuclearpasta / react-native-drax

A drag-and-drop system for React Native
MIT License
554 stars 69 forks source link

add wrapper styles for DraxList #41

Closed chrisdrackett closed 3 years ago

chrisdrackett commented 4 years ago

fixes #40

lafiosca commented 4 years ago

Review of this is pending my experimentation per https://github.com/nuclearpasta/react-native-drax/issues/40#issuecomment-645110022

chrisdrackett commented 4 years ago

any update on this? we're currently using our own fork, but it would be nice to be able to switch back to the official package

lafiosca commented 4 years ago

Sorry, @chrisdrackett. I've been remiss in working on Drax the past couple months, and I do not have an update yet. I just completed a major milestone in my day work last week, and I will try to carve out a little more time soonish to return some attention here as I return to working on our mobile app.

chrisdrackett commented 4 years ago

@lafiosca curious why this was deleted? is this possible via other means now?

lafiosca commented 4 years ago

@chrisdrackett This and #36 were accidentally, automatically closed yesterday when I deleted the prerelease branch (renamed to main). It appeared GitHub provided no way to reopen them or change the base branch while it didn't exist, so I have recreated it just to reopen these issues and point them to the right place. Sorry about that.

In other news: my work has finally returned me to this library, so I hope to take care of some of these outstanding changes over the next week or two.

lafiosca commented 3 years ago

@chrisdrackett I'm sorry for taking forever to get back to this. Life has been life. I'm currently going through a burst of maintenance on the library, and I am trying to revisit all of the PRs at a minimum.

Am I reading this change correctly in that it would pass the style prop down to the underlying FlatList and then apply the new wrapperStyles prop to the outer View? If so, I would consider this to be a breaking change, and I think I would rather see a new prop which is fed to the FlatList (perhaps listStyle?), leaving the style prop where it is on the top-level View. I am open to discussion on this if you have reasons for preferring the other approach.

chrisdrackett commented 3 years ago

I think I suggested using style because in general this component acts as a stand-in for the native FlatList component and shares many of the props.

Regardless, I'm not super set on what these are named, we just need a way to style both of these elements.

lafiosca commented 3 years ago

Ah yea, to me this gets very philosophical, even raising questions about how ideally I would prefer Drax to work, even if it's not entirely feasible. But since you don't have a strong preference, I lean toward the non-breaking change. Additionally, here is some of my deeper reasoning:

In cases like ScrollView or FlatList where a component composes multiple Views, often there are props exposed to style the different parts. Generally speaking, it seems like the top-level style prop should refer to the highest level outer view, as it will be what's subject to layout rules. Specifically, both of those mentioned offer contentContainerStyle to style the implementation view inside of them. Along similar lines, I don't think we can pretend that DraxList is a true replacement for FlatList. It's clear that it's a DraxView monitor wrapping a FlatList. From that perspective I think style should apply to the outer DraxView, and I'll add flatListStyle for the inner FlatList.

lafiosca commented 3 years ago

Superseded by #91