Inserter - show Picker with registered blocks to choose from #112
Conversation
…ypes when inserting new block
|
@koke This code has already been reviewed / approved in #105 except for this particular line here https://github.com/wordpress-mobile/gutenberg-mobile/pull/112/files#diff-2d50daf729e068ecebcce113d1ce7f9aR92 which has been added after the fix in #108. Would you like to give it a quick look and re-approve, or otherwise make any comments as you see fit? Thanks in advance 🙇 |
mzorz
changed the base branch from
try/inserter-take1-state-props-direct-insert-take2
to
feature/inserter
| newBlock.focused = false; | ||
|
|
||
| // set it into the datasource, and use the same object instance to send it to props/redux | ||
| this.state.dataSource.splice( focusedItemIndex + 1, 0, newBlock ); |
There was a problem hiding this comment.
Aren't you supposed to modify state only by calling setState? I still have a lot to learn about React and I'm not sure which side effects this might have.
There was a problem hiding this comment.
I know, right? 😉 That's exactly my understanding from my learnup that ended in the elaborate description of #95. But, one of the troublesome things that appear here is that the list will redraw itself on each state change as per React's mandate, which is something we still have trouble digesting (feels super counterintuitive after having learnt to deal with recycled views and making good use of list updates for years). Also this kills the native animations when moving blocks, which was something that was tested early on when picking the List component, as ultimately we want to have a best UX on Gutenberg when moving blocks around.
Long story short, we are keeping the same state on both sides (RecyclerView and redux) to make sure we can still use the fine grained RecyclerView API to move items around and get native animations to work, without incurring in heavy lifting. More info in #108.
There was a problem hiding this comment.
Got it. I'm going to go with 👍 for now since I don't want to be blocking on this, but it seems to me as if the recyclerlist library wasn't implemented in a way that's consistent with React's declarative style. If I understood the code correctly, the list isn't implementing shouldComponentUpdate, and the README seems to imply that you should update the contents by manipulating the data source directly.
What I would expect is that you'd call setState and the list would deal with calculating what changed from the previous one and what needs to be inserted/deleted/updated. But this is what we have for now 🤷♂️
There was a problem hiding this comment.
On the same boat here 😄. We could set some time apart to further investigate and see if using a FlatList and non-native animations makes the cut for both platforms and eventually go with it, making good use of the React pattern 👍
| newBlock.focused = false; | ||
|
|
||
| // set it into the datasource, and use the same object instance to send it to props/redux | ||
| this.state.dataSource.splice( focusedItemIndex + 1, 0, newBlock ); |
There was a problem hiding this comment.
Got it. I'm going to go with 👍 for now since I don't want to be blocking on this, but it seems to me as if the recyclerlist library wasn't implemented in a way that's consistent with React's declarative style. If I understood the code correctly, the list isn't implementing shouldComponentUpdate, and the README seems to imply that you should update the contents by manipulating the data source directly.
What I would expect is that you'd call setState and the list would deal with calculating what changed from the previous one and what needs to be inserted/deleted/updated. But this is what we have for now 🤷♂️
…l-position Add onCaretVerticalPositionChange event
Supersedes already reviewed/approved #105 to clean commit history
This completes a very draft, initial 1st iteration on implementing the inserter flow. With this, the flow goes roughly along the lines of the expected actions like this:
+button to insert a new item (this+trigger will be moved to the toolbar as per the designs Inserter #58 (comment))+makes a RNPickershow upKnown issues:
Pickercomponent, though easy to use, as far as I could check only reacts when an item other than the currently selected value is chosen, and has had variable behavior in the recent past:Android Picker Not Consistently Firing onValueChange facebook/react-native#15556
Picker's onValueChange is not fire when picker first item facebook/react-native#15672
Can't select first option from Picker facebook/react-native#13204
https://stackoverflow.com/questions/48465288/detect-selecting-current-selectedvalue-with-react-native-picker
Kind of makes sense with the prop name
onValueChange, (i.e. I think from the name itself it derive that if the value didn't change, it won't be triggered) although it's not exactly what can be expected of the control (even the docs describe the prop function asCallback for when an item is selected.)For this I'm working on another implementation with a modal dialog and a flatlist, but wanted to continue to put this PR up and complete a first "working" flow at least.