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(reverseHighlight/reverseSnippet): Implements reverseHighlight and reverseSnippet #4592

Merged
merged 22 commits into from
Dec 23, 2020

Conversation

shortcuts
Copy link
Member

@shortcuts shortcuts commented Nov 26, 2020

This implements reverseHighlight and reverseSnippet.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 26, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit a88b614:

Sandbox Source
InstantSearch.js Configuration

@shortcuts shortcuts requested a review from a team November 26, 2020 15:20
@ghost ghost requested review from Haroenv and yannickcr and removed request for a team November 26, 2020 15:20
.storybook/static/storybook.css Outdated Show resolved Hide resolved
.storybook/static/storybook.css Outdated Show resolved Hide resolved
src/helpers/reverseHighlight.ts Outdated Show resolved Hide resolved
src/lib/utils/__tests__/getReversedHighlight-test.ts Outdated Show resolved Hide resolved
src/lib/utils/getReversedHighlight.ts Outdated Show resolved Hide resolved
@shortcuts
Copy link
Member Author

shortcuts commented Dec 1, 2020

I ran into a problem with reverseHighlighting and non alphanumeric characters where I didn't find which logic to follow. I expose below the 3 method I came across so I can get your thoughts on that.

Could you please let me know what you think/which way is the best for you?

Sorry for the low quality post

  1. The actual behavior that autocomplete follows, which leaves any non alphanumeric character bold.
    actual_behavior

  2. The sibling behavior, where the non alphanumeric character will take the highlighted value of the previous and next element if they match, reverse it if they don't.
    sibling_behavior

  3. The highlight behavior, where it just escape any non alphanumeric characters like the current highlight method.
    highlight_behavior

cc @algolia/instantsearch-for-websites

@Haroenv
Copy link
Contributor

Haroenv commented Dec 1, 2020

Thanks for trying out all those options @shortcuts, I'm a fan of the method #2's output, and I think it would be better for highlight itself too.

@francoischalifour
Copy link
Member

francoischalifour commented Dec 4, 2020

@shortcuts The "sibling behavior" version is interesting. The only place I find it a bit weird is:

(Model)

[...] where the last ) is bold. It makes me wonder is never highlighting non-alphanumeric characters would make sense.

Have you seen this sibling pattern anywhere else?

@shortcuts
Copy link
Member Author

shortcuts commented Dec 4, 2020

@francoischalifour I've never seen this pattern used before and this behavior is the only reason why I'm not sure about using it.

I could think of a workaround for the parenthesis where if something inside is highlighted, we remove the highlight on the opening/closing one, but that would leave some characters bold

Before After
(ModelExample)
(ModelExample)

The thing is that it would add even more computation on top of what's already done

@Haroenv
Copy link
Contributor

Haroenv commented Dec 7, 2020

I think the sibling behaviour in that case is actually correct, since you are matching on the rest of the sentence, including that closing parenthesis. I think (ModelExample) looks more "pay attention from here onwards", while (ModelExample) looks like "pay only attention to 'lExample'", which will be odd if there's other highlighted text after this word.

Google's autocomplete seems to try following a sibling behaviour, although it often sees separators as typos and won't highlight the match

@shortcuts
Copy link
Member Author

You're right, they are also doing it.

I didn't find a good "long sentence in parentheses" to do more tests but that's already a good point.

Screenshot 2020-12-07 at 10 05 56

@shortcuts shortcuts marked this pull request as ready for review December 9, 2020 08:54
Copy link
Member

@francoischalifour francoischalifour left a comment

Choose a reason for hiding this comment

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

Looks good—great job!

Since the bundlesize integration is disabled right now, what's the bundle size increase of this feature?

src/lib/utils/unescape.ts Show resolved Hide resolved
@shortcuts
Copy link
Member Author

Here's what I got for the bundle size

 FAIL  ./dist/instantsearch.production.min.js: 65.29KB > maxSize 65.25KB (gzip)
 PASS  ./dist/instantsearch.development.js: 151.83KB < maxSize 160KB (gzip)

@francoischalifour
Copy link
Member

You can increase it to 65.5KB then.

src/lib/utils/__tests__/concatHighlightedParts-test.ts Outdated Show resolved Hide resolved
src/lib/utils/__tests__/getHighlightedParts-test.ts Outdated Show resolved Hide resolved
src/lib/utils/__tests__/reverseHighlightedParts-test.ts Outdated Show resolved Hide resolved
src/lib/utils/getHighlightFromSiblings.ts Outdated Show resolved Hide resolved
src/lib/utils/getHighlightFromSiblings.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

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

looks good!

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