Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

NVRC
Copy link

@NVRC NVRC commented Nov 24, 2022

No description provided.

@NVRC
Copy link
Author

NVRC commented Nov 24, 2022

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Author

NVRC commented Nov 24, 2022

@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
Copy link
Owner

@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
Copy link
Author

NVRC commented Nov 24, 2022

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 NVRC marked this pull request as draft November 24, 2022 23:44
@Kieran-McIntyre
Copy link
Owner

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 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants