search

jwohlfert23 / react-native-tag-input

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

Modernize react-native-tag-input #36

Closed Ashoat closed 6 years ago

Ashoat commented 7 years ago

I'm using this library in my application, and ended up "modernizing" it a bit. Changes include:

  1. Using prop-types package (fixes bug in package for >RN0.45)
  2. Refine various Flow types, ie. no more Object, use StyleObj from react-native, use $ReadOnlyArray, etc.
  3. Replace all measure() calls (which break on Android - x-position is always 0 for nested inline elements) with onLayout (more idiomatic)
  4. Use PureComponent
  5. Avoid props recalculation by getting rid of all inline function declarations in props
  6. Create new Tag component to avoid having to do inline function declarations
  7. And several others...

I recognize this is a pretty meaty PR, and I considered not submitting it since I imagine you might ask for it to broken into smaller PRs, but I figured there's nothing to lose by submitting :)

Ashoat commented 7 years ago

Looking at this a bit more, there are several more opinionated changes, including changes to default component style and code formatting. Let me know if you'd like me to revert those portions and I can try to come up with something that includes just the "modernization" pieces. Mostly just wanna know that PRs are welcome before doing the work :)

rtman commented 7 years ago

@sibelius @jwohlfert23 Just wondering if this is going to be reviewed, I would like to use this package but not if it isn't up to date with the current state of react native. It looks really useful!

Ashoat commented 7 years ago

Sorry about the style changes etc. in the initial PR! Like 50% of Github repo maintainers don't pay attention to PRs, so I've learned not to invest too much into a PR before I get a response. I've just updated the PR with several other modernizations, and the opinionated stuff should be out.

I should note that I'm using this component as a typeahead (eg. search results appear below the TagInput, and update whenever the text changes). To make that happen I had to fork a version with a callback for when the text changes. I haven't included those changes here, since they're orthogonal to the modernization effort, but if you're open to it I'd like to put up a PR with those changes once these ones are in.

rtman commented 6 years ago

I'd like to see that typeahead get merged too

Sent from my Google Nexus 5 using FastHub

Ashoat commented 6 years ago

@rtman - to be clear, it's not a full typeahead; just a refactored version of this component that can be used to build a typeahead.

I went ahead and created an RFC issue to discuss merging that version into this one: #39. However, note that it would represent pretty substantial changes, most notably ripping out the existing parseTags logic. Consequently, figured I'd solicit comments on the change. There's a link in the issue to the version of this component I am using, so you can just copy that version into your project if you'd like.