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(toggleRefinement): implement canRefine #4689

Merged
merged 1 commit into from
Mar 30, 2021

Conversation

eunjae-lee
Copy link
Contributor

@eunjae-lee eunjae-lee commented Mar 22, 2021

Summary

implements canRefine in connectToggleRefinement as true when there value.count exists.

DX-1309

Result

toggleRefinement now has a canRefine

@codesandbox-ci
Copy link

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 846a826:

Sandbox Source
InstantSearch.js Configuration

@@ -306,6 +307,7 @@ export default function connectToggleRefinement(renderFn, unmountFn = noop) {
createURL,
}),
sendEvent,
canRefine: Boolean(results ? nextRefinement.count : null),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what condition it should be here. Maybe to be false, but current refinement and next refinement should have a 0 count? I'm not sure!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This condition is actually coming from Vue InstantSearch

const mapStateToCanRefine = state => Boolean(state.value && state.value.count);

and state.value is made in InstantSearch.js like this:

          value: {
            name: attribute,
            isRefined,
            count: results ? nextRefinement.count : null,
            onFacetValue,
            offFacetValue,
          },

I'm not too sure either, but it'd be good to align between InstantSearch.js and Vue InstantSearch first, and then later change to something else if needed from the InstantSearch.js and propagate to Vue InstantSearch. What do you think?

@eunjae-lee eunjae-lee requested review from Haroenv and tkrugg March 30, 2021 09:25
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.

good enough for now

@Haroenv Haroenv merged commit 48dc7f8 into master Mar 30, 2021
@Haroenv Haroenv deleted the feat/canrefine-toggle-refinement branch March 30, 2021 11:20
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