jwohlfert23 / react-native-tag-input

A simple React Native component that creates an input for tags, emails, etc.
MIT License
231 stars 104 forks source link

Support scrollHorizontal and other scrollViewProps #44

Closed JaxGit closed 6 years ago

JaxGit commented 6 years ago

Considerations: It's good when scrolling horizontally to have the similar behaviour as when scrolling vertically

  1. Deleting a previous tag (removeIndex) shouldn't trigger scrolling to end (to maintain visibility of adjacent tags);
  2. Deleting the last tag (‘Backspace’ on iOS, maybe on Android in the future) should trigger scrolling to end, especially if the textInput has been scrolled out of sight;
  3. Adding a new tag should trigger scrolling to end (to see the newly added tag), especially when the input area is previously being scrolled out of sight.

The vertical scroll behaviours has been handled by onContentSizeChange, I guess it makes sense especially when some existing tags and textInput are in the same line, and textInput contentSize has grown outside the right edge and been flex wrapped to the next line. However it seems to still have some minor issues like not on Android but iOS sometimes it’s overscrolling down while some other times it doesn’t start scrolling even the last line is partly covered.

While the horizontal scroll case is a bit simpler, when textInput contentSize has exceeded, even Instagram is keeping the position of existing tags instead of doing endless scroll to the end, so the user will be always safe to see those tags, and frankly the searching string entered usually doesn’t need to be so long. Also scrollView.scrollToEnd() has been supported since RN 0.42. So it’s simpler to check and use scrollToEnd for the above 3 events instead of checking in onContentSizeChange.

P.S. As mentioned by @Ashoat in #42 and shown in text lifting PR #43 , it’s better for the parent to manage and add new tag (like in onChangeText, autocompletion or multi-selection components). So after #43 has been merged, I guess in order to handle event 3, the user could still find the scrollToRight() helpful as a public helper function just like ScrollView did.

I also eliminated my old refactors in #42 for easier merge of possible incoming text lifting PR. Please fell free to refactor later whenever appropriate.

JaxGit commented 6 years ago

Will check these soon.

JaxGit commented 6 years ago

Please wait for me to fix flow errors..

JaxGit commented 6 years ago

Excuse me @Ashoat , apart from my typo scrollViewProps?: , it seems that:

  1. By defining scrollViewProps: PropTypes.shape(ScrollView.propTypes), I also encountered similar Flow error as for inputProps, not only EdgeInsetsPropType, but also PointPropType is not identified as React PropType, so I just suppress it;

  2. By defining scrollViewProps?: $PropertyType<ScrollView, 'props'>, , Flow start to complain boolean literalfalseThis type is incompatible with string. 2 possible ways to remove this error are to delete default keyboardShouldPersistTaps="handled" or to define scrollViewProps?: Object, I wonder if this is another bug of Flow... as I am not so familiar of it, I would be glad if you could help to confirm. Thanks!

Ashoat commented 6 years ago

Thanks for taking the time to do this @JaxGit! I'll try to publish a new package today.

Ashoat commented 6 years ago

New version 0.0.18 published!

JaxGit commented 6 years ago

Thank you @Ashoat , glad to contribute, I've also learnt a lot 😄