shirakaba / react-nativescript

React renderer for NativeScript
https://react-nativescript.netlify.com
MIT License
284 stars 14 forks source link

Revisit event attachment/detachment #43

Closed shirakaba closed 5 years ago

shirakaba commented 5 years ago

I haven't thought about this for a while and have forgotten what's going on.

1) Slider doesn't implement updateListeners():

https://github.com/shirakaba/react-nativescript/blob/master/react-nativescript/src/components/Slider.ts

2) "submit" and "close" are implemented in SearchBar's updateListeners, but only "textChange" is mentioned in its componentDidMount and componentWillUnmount methods.

https://github.com/shirakaba/react-nativescript/blob/master/react-nativescript/src/components/SearchBar.ts

I'd expect Page, View, and EditableTextBase to be reliable prototypes to refer to:

https://github.com/shirakaba/react-nativescript/blob/master/react-nativescript/src/components/Page.ts

https://github.com/shirakaba/react-nativescript/blob/master/react-nativescript/src/components/View.ts

https://github.com/shirakaba/react-nativescript/blob/master/react-nativescript/src/components/EditableTextBase.ts

I'll look into them when I have a moment.

Lelelo1 commented 5 years ago

TabView onSelectedIndexChange isen't firing: https://github.com/shirakaba/react-nativescript/blob/master/react-nativescript/src/components/TabView.ts

Using the nativescript onSelectedIndexChanged method of the component fires undefined - and causes an issue with TabViewItem content not being shown.

Lelelo1 commented 5 years ago

I'd expect Page, View, and EditableTextBase to be reliable prototypes to refer to:

So the stuff in componentDidMount and ComponentWillUnmount in DatePicker and Slider for instance - should be in updateListeners?

shirakaba commented 5 years ago

Good catch. That might be down to a typo; I should name it onSelectedIndexChange rather than onSelectedIndexChanged as per Triniwiz's advice in #26.

I'll make that fix on master for now.

EDIT: done in e46cb1f.

shirakaba commented 5 years ago

So the stuff in componentDidMount and ComponentWillUnmount in DatePicker and Slider for instance - should be in updateListeners?

Yes, I believe so. Originally, I did everything separately across componentDidMount, componentWillUnmount, and shouldComponentUpdate for all components, but now this is abstracted by Observable into one convenient (yet somewhat unintuitive) updateListeners method to reduce code repetition.

Lelelo1 commented 5 years ago

Good catch. That might be down to a typo; I should name it onSelectedIndexChange rather than onSelectedIndexChanged as per Triniwiz's advice in #26.

SegmentedBar currently has i'ts prop named: onSelectedIndexChanged

shirakaba commented 5 years ago

Thanks; fixed in: 61f80e9 (will create release later).

shirakaba commented 5 years ago

I've now released v0.13.0.

Event handlers now return NarrowedEventData rather than the value concerned (e.g. no longer return time for onTimeChange but simply args); reverting to this simpler pattern reduces code size and was a simple way to fix such event handlers so that they can now be updated during component's lifecycle.

I reckon I've fixed TabView. Some components in NativeScript Core exceptionally use onSelectedIndexChanged while others use onSelectedIndexChange, which confused things.