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: add indexOf method for useFilter #7174

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

feat: add indexOf method for useFilter #7174

wants to merge 6 commits into from

Conversation

qmhc
Copy link

@qmhc qmhc commented Oct 11, 2024

Add indexOf method for useFilter.

I have met a situation where it is necessary to filter options by the index of a substring, so it would be better if an indexOf method could be provided directly.

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

Currently I am using the following as a workaround:

const collator = useCollator({ usage: 'search', sensitivity: 'base' });
const indexOf = useCallback((string: string, substring: string) => {
  if (substring.length === 0) {
    return 0;
  }

  string = string.normalize('NFC');
  substring = substring.normalize('NFC');

  const sliceLen = substring.length;
  const strLen = string.length;
  for (let scan = 0; scan + sliceLen <= strLen; scan++) {
    const slice = string.slice(scan, scan + sliceLen);
    if (collator.compare(substring, slice) === 0) {
      return scan;
    }
  }

  return -1;
}, [collator]);

@snowystinger
Copy link
Member

Thanks for the PR, you'll need to sign the CLA https://react-spectrum.adobe.com/contribute.html#contributor-license-agreement then you may need to close and reopen the PR to get it to take

@qmhc qmhc closed this Oct 23, 2024
@qmhc qmhc reopened this Oct 23, 2024
Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a reasonable request.
Giving a provisional approval and will bring up with the rest of the team.

@reidbarber
Copy link
Member

Discussed with the team and the one concern we have is that it doesn't match the existing pattern of the filter functions which is to return a boolean. Do you mind sharing the use case you ended up needing this for? We may need something like this in the future, so it may help us figure out where it could fit in.

@qmhc
Copy link
Author

qmhc commented Nov 22, 2024

@reidbarber Perhaps this is a useSorting scenario.

In my case, there are some date and time options that need to be sorted by the first match position while being searched. The options that match earlier should be placed in front.

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.

3 participants