Kieran-McIntyre / react-native-section-alphabet-list

A simple React Native component that takes an array of data and renders a SectionList with alphabetically sorted data
MIT License
189 stars 55 forks source link

feat(types): change IData.value type to generic #55

Open NVRC opened 1 year ago

NVRC commented 1 year ago

As far as I can tell the commit looks good. Lmk if more testing pipeline stuff needs doing.

$ npm run test && npm run lint && npm run build

> react-native-section-alphabet-list@2.1.0 test
> jest --config jest.config.js

 PASS  src/utils/getSectionData.test.ts
 PASS  src/components/ListLetterIndex/index.test.tsx
 PASS  src/components/AlphabetList/index.test.tsx

Test Suites: 3 passed, 3 total
Tests:       18 passed, 18 total
Snapshots:   0 total
Time:        3.048s
Ran all test suites.

> react-native-section-alphabet-list@2.1.0 lint
> eslint

> react-native-section-alphabet-list@2.1.0 build
> rm -rf dist && tsc
Kieran-McIntyre commented 1 year ago

As far as I can tell the commit looks good. Lmk if you more testing pipeline stuff needs doing.

$ npm run test && npm run lint && npm run build

> react-native-section-alphabet-list@2.1.0 test
> jest --config jest.config.js

 PASS  src/utils/getSectionData.test.ts
 PASS  src/components/ListLetterIndex/index.test.tsx
 PASS  src/components/AlphabetList/index.test.tsx

Test Suites: 3 passed, 3 total
Tests:       18 passed, 18 total
Snapshots:   0 total
Time:        3.048s
Ran all test suites.

> react-native-section-alphabet-list@2.1.0 lint
> eslint

> react-native-section-alphabet-list@2.1.0 build
> rm -rf dist && tsc

Thanks @NVRC! This looks good, will get it merged and will release a new version. The new version will be v2.1.1 👍

Kieran-McIntyre commented 1 year ago

As far as I can tell the commit looks good. Lmk if you more testing pipeline stuff needs doing.

$ npm run test && npm run lint && npm run build

> react-native-section-alphabet-list@2.1.0 test
> jest --config jest.config.js

 PASS  src/utils/getSectionData.test.ts
 PASS  src/components/ListLetterIndex/index.test.tsx
 PASS  src/components/AlphabetList/index.test.tsx

Test Suites: 3 passed, 3 total
Tests:       18 passed, 18 total
Snapshots:   0 total
Time:        3.048s
Ran all test suites.

> react-native-section-alphabet-list@2.1.0 lint
> eslint

> react-native-section-alphabet-list@2.1.0 build
> rm -rf dist && tsc

Thanks @NVRC! This looks good, will get it merged and will release a new version. The new version will be v2.1.1 👍

Sorry, I spoke a little too soon. We're seeing some failing tests, but doesn't look related to your PR. I have a PR to fix this, so will merge my fixes, then you will need to merge master into your branch. Please give me a few minutes to do this.

NVRC commented 1 year ago

@Kieran-McIntyre No rush. I jumped the gun a little with the PR anyways since I didn't add a test case for the complex obj. I'll do so later tonight if you want to hold off.

Kieran-McIntyre commented 1 year ago

@Kieran-McIntyre No rush. I jumped the gun a little with the PR anyways since I didn't add a test case for the complex obj. I'll do so later tonight if you want to hold off.

@NVRC Okay no worries. I've pushed a new version to master (v3.0.0) if you want to update your PR and add tests etc

NVRC commented 1 year ago

A side effect of this change is that the default sorting behaviour needs some rethinking. With the current assumption that value is a string the library uses it's first character as the index on which to sort.

With another prop we can maintain the existing behaviour and let users define a function to extract the sortable index. This works like a keyExtractor in libraries that consume lists as opposed to maps. I think this is your intention here.

Proposal

... AlphabetListProps ...
  indexExtractor?: (item: IData<Type>) => string;

Default indexExtractor extracts value for backwards compat.

@Kieran-McIntyre Thoughts?

Kieran-McIntyre commented 1 year ago

A side effect of this change is that the default sorting behaviour needs some rethinking. With the current assumption that value is a string the library uses it's first character as the index on which to sort.

With another prop we can maintain the existing behaviour and let users define a function to extract the sortable index. This works like a keyExtractor in libraries that consume lists as opposed to maps. I think this is your intention here.

Proposal

... AlphabetListProps ...
  indexExtractor?: (item: IData<Type>) => string;

Default indexExtractor extracts value for backwards compat.

@Kieran-McIntyre Thoughts?

@NVRC Yes, this looks like a good proposal and would resolve the issue. We will also need to update the README to clarify how this should be used. But happy to go with this 👍