Addition of multi-select feature. #127
Conversation
|
Thanks @allroundexperts I would recommend against including react-native-vector-icons in this library because that requires native linking. Not everyone who uses this library may want to do that, or use that particular icon library. It would be better to design this so that the user can pass in a component (which could include an icon) instead of building that in. |
@peacechen Fixed in the latest commit! |
|
👍 |
Yup! |
|
I'll have time to test this soon. A few more items need to be addressed: Some of the props/methods have changed names ( Tab spacing in some chunks of code have changed from 2 to 4 spaces. The eslintrc should specify tab size of 4 to prevent commits that only change tab spacing (and resultant swaths of diffs that have no functional changes). |
index.js
Outdated
| if (this.props.multiple) | ||
| checked | ||
| ? selected.push(itemKey) | ||
| : selected.indexOf(itemKey) >= 0 && | ||
| delete selected[selected.indexOf(itemKey)]; | ||
| else selected = [itemKey]; |
There was a problem hiding this comment.
This conditional without braces makes the code harder to read at first glance. It will also be more prone to bugs if someone modifies it later. Please add braces for both the if and else sections.
index.js
Outdated
| selectedKey: '', | ||
| initSelectedKeys: [], | ||
| multiple: false, | ||
| renderCheckbox: undefined, |
There was a problem hiding this comment.
The Readme says that renderCheckbox is required (not optional). However it's not needed for single select mode, only multi select. Please provide a default empty method for renderCheckbox () => {}, update the Readme to reflect that the prop is optional, and in the description note that the user should provide a render method if they want checkboxes to display.
- Added multi select option. - Added husky, making auto linting possible. - Upgraded RN to the latest version.
allroundexperts
force-pushed
the
master
branch
from
1d4d858 to
cc18e96
Compare
This reverts commit ac29629.
|
@allroundexperts The sample app in master has been updated to a simpler Expo project. Please revert your changes in SampleApp, keeping only the changes in App.js and any dependencies added. You're using react-native-elements in the SampleApp which may need tweaks to the Expo project (or maybe not, most of that is automated now). It would be cleanest to revert everything in SampleApp, pull from master and rebase & squash your commits to the top of Head. It's a bit difficult to see the big picture in this PR when there are ~20 commits. Would you mind squashing them into a single commit? |
PS: Messed up the previous PR, so consider this please.