Skip to content

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

Draft
NVRC wants to merge 1 commit into
Kieran-McIntyre:masterfrom
NVRC:feature/#54-convert-IData-value-to-generic
Draft

feat(types): change IData.value type to generic#55
NVRC wants to merge 1 commit into
Kieran-McIntyre:masterfrom
NVRC:feature/#54-convert-IData-value-to-generic

Conversation

@NVRC
Copy link
Copy Markdown

@NVRC NVRC commented Nov 24, 2022

No description provided.

@NVRC
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

2 participants