Skip to content
This repository has been archived by the owner on Dec 30, 2022. It is now read-only.

docs(refining-hits): add a tutorial on how to refine within hits #258

Closed
wants to merge 25 commits into from

Conversation

Haroenv
Copy link
Contributor

@Haroenv Haroenv commented Aug 14, 2017

this is a guide that finishes with https://codesandbox.io/embed/oY1klpZYB

fixes #244 and is obviously quite advanced

can be found in the deploy preview under guide/Refining_hits.html

@Haroenv Haroenv requested review from mthuret and marielaures August 14, 2017 16:00
mainTitle: Guides
layout: main.pug
category: guide
navWeight: # idk what to put here 🚨
Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do we put here?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's the number that allow you to positionate the guide in the sidebar. If you want it at the end, it should be bigger than the last one (not just by one to let other guide to be in between).

@algobot
Copy link
Contributor

algobot commented Aug 14, 2017

Deploy preview for react-instantsearch ready!

Built with commit b566919

https://deploy-preview-258--react-instantsearch.netlify.com

@Haroenv
Copy link
Contributor Author

Haroenv commented Aug 14, 2017

Copy link
Member

@rayrutjes rayrutjes left a comment

Choose a reason for hiding this comment

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

I made a few comments here and there.
Impressive stuff.


## Regular setup

First, we'll set up a regular project wit create-react-app and react-instantsearch.
Copy link
Member

Choose a reason for hiding this comment

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

Wit => with

});
```

An index that already has these settings is the index for search on [Yarn](https://yarn.fyi), which is used in this guide. We will get started in `App.js` and set up a regular React InstantSearch setup:
Copy link
Member

Choose a reason for hiding this comment

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

In this guide we will use the search index we use on Yarn. It already has the mandatory attributesForFaceting configured.
We will get started in App.js and set up a regular React InstantSearch app:

export default Hit;
```

In this component we took the name, description and owner of a certain package, and render these as hits. To enable this as HitComponent, we will change the line in `App.js` that renders the `Hit` component:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe quote HitComponent?
Weird tense switch here. I would stay with the present everywhere.

export default App;
```

We manually render the `hitComponent` as a function, because we want to add extra functions to the `Hit` component later. Now we will also set up two `RefinementList`s for `keywords` and `owner.name`. We do this in a container so that the difference is clear, but obviously you can change the rendering of this however you prefer:
Copy link
Member

Choose a reason for hiding this comment

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

Remove obviously

export default VirtualRefinementList;
```

Now this seems pretty useless, except that this component will get the correct props from its connector, each time something in its props change. The props we get from this connector are: `currentRefinement` and `defaultRefinement`. `defaultRefinement` is what we will manually set from within our Hit, `currentRefinement` is the current state of the search, relevant to that refinement, but maybe we should first implement it in our `App.js`:
Copy link
Member

Choose a reason for hiding this comment

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

change => changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the version that’s written here is the correct version

export default App;
```

We will change the state of `App` from within the `Hit` component later so that it will update the `defaultRefinement` of each respective `VirtualRefinementList`. To be able to handle with that cleanly, let's add a lifecycle hook in `VirtualRefinementList`:
Copy link
Member

@rayrutjes rayrutjes Aug 14, 2017

Choose a reason for hiding this comment

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

To be able to achieve that in a clean way?

}
```

We compare the current refinement with the previous current refinement. If they aren't the same, then we will call a callback `onRefine` from our props with the current `attributeName` and with the new value. We do the same for the `defaultRefinement` so that the correct refinement is always chosen.
Copy link
Member

Choose a reason for hiding this comment

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

then we call the onRefine callback

}
```

This function works as follows: it has a `attributeName` and `value`, the value is what we want to add to the `VirtualRefinementList`, a string, for example `nodejs` or `react`. We will first check if our array for this attribute already contains that value. If it doesn't contain it, we simply add it to the array, if the array already contains the value we want, we remove it. Finally this value is set as the new state. Correspondingly, that change in state will trigger a change in `defaultRefinement` of the `VirtualRefinementList` for that attribute, and update the search interface.
Copy link
Member

Choose a reason for hiding this comment

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

  • it has an attributeName and a value.
  • remove the simply


This function works as follows: it has a `attributeName` and `value`, the value is what we want to add to the `VirtualRefinementList`, a string, for example `nodejs` or `react`. We will first check if our array for this attribute already contains that value. If it doesn't contain it, we simply add it to the array, if the array already contains the value we want, we remove it. Finally this value is set as the new state. Correspondingly, that change in state will trigger a change in `defaultRefinement` of the `VirtualRefinementList` for that attribute, and update the search interface.

But we still need to call this function. What we do is add `this.refine` as a prop to the `Hit` component, and call it there when needed. The inside of the `Hit` component now looks like this:
Copy link
Member

Choose a reason for hiding this comment

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

The Hit component implementation now looks like this:

@mthuret
Copy link
Contributor

mthuret commented Aug 16, 2017

If someone only wants the list of tags on Hits without the other RefinementList does it simplify things?

Copy link
Contributor

@mthuret mthuret left a comment

Choose a reason for hiding this comment

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

Awesome work @Haroenv, that's a great use case added to RIS :)


## Introduction

In some interfaces, you want to have buttons _inside_ your Hits that will change the refinement of the current search, rather than as standalone components. Since we can't replicate this pattern with widgets, we will need to go with Refinementlists that don't render anything. Please note that this is an advanced tutorial and requires moderate knowledge of how React and React InstantSearch work. Our final result will look something like this, let's get started!
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should start directly by evoking the "List of tags" use case that speaks easily to somebody.


## Introduction

In some interfaces, you want to have buttons _inside_ your Hits that will change the refinement of the current search, rather than as standalone components. Since we can't replicate this pattern with widgets, we will need to go with Refinementlists that don't render anything. Please note that this is an advanced tutorial and requires moderate knowledge of how React and React InstantSearch work. Our final result will look something like this, let's get started!
Copy link
Contributor

Choose a reason for hiding this comment

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

Refinementlists => RefinementLists

Mention Virtual Widgets and link to the guide

In the end we should have a clear prerequisites list.

$ yarn add react-instantsearch # or npm install --save react-instantsearch
```

The index that we will use in this example needs to be set up with:
Copy link
Contributor

Choose a reason for hiding this comment

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

This part is confusing. The code sample uses an index that is already configured. If the user wants to use his own he will not have the same attribute name. I guess this should be rephrase a bit.


## Adding virtual RefinementLists

A "virtual" RefinementList is a RefinementList, just like the ones that are using the regular widgets, but with the exception that they won't render anything. We will make a virtual RefinementList here that will take `defaultRefinement` into account when rendering. We will make a `VirtualRefinementList` component, which uses a `connectRefinementList`:
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we should introduce this paragraph by explaining why we need virtual widgets in the first place

@Haroenv
Copy link
Contributor Author

Haroenv commented Aug 17, 2017

You mean just keywords and not owner.name? Yes, that’s a bit more simple, but was pretty limiting. I like this solution better, since it’s simpler in the end. In the Yarn website it’s still using the “there can only be one attribute” way of doing things, and will probably change that to what I described here.

Showing the real refinement list doesn’t make it more or less complex

@mthuret
Copy link
Contributor

mthuret commented Aug 18, 2017

I was talking about this: "Showing the real refinement list doesn’t make it more or less complex".

@Haroenv
Copy link
Contributor Author

Haroenv commented Aug 18, 2017

Only difference is when you show the real refinementList that there are some errors in the console

@mthuret
Copy link
Contributor

mthuret commented Aug 28, 2017

Additional notes: I played with it a bit, it works like a charm except that it produces 3-4 query per selection. Do we have a way to optimize this @Haroenv?

@Haroenv
Copy link
Contributor Author

Haroenv commented Sep 6, 2017

Hmm, should look at that issue @mthuret, thanks for noticing! I wonder what the cause would be, since it should just be another refinementList

@Haroenv
Copy link
Contributor Author

Haroenv commented Sep 19, 2017

It's the virtualRefinementList that causes the queries, the actual widgets being there doesn't matter there has no effect

@mthuret
Copy link
Contributor

mthuret commented Sep 20, 2017

@Haroenv did you had the chance to understand why? Can we fix it? I'm not comfortable with shipping a guide that does all those extra queries and I'd rather try to find a solution that is cleaner. It might involved dedicated dev if nothing possible using React capabilities.

@Haroenv
Copy link
Contributor Author

Haroenv commented Sep 20, 2017

No, I didn't find the actual reason yet, it doesn't seem easy to find out what the cause of the queries are. See also #355

@vvo
Copy link
Contributor

vvo commented Sep 26, 2017

Quick note, is the status on this PR pending while we find reasons for #355?

@Haroenv
Copy link
Contributor Author

Haroenv commented Sep 26, 2017

yep, this PR is blocked by #355

@vvo
Copy link
Contributor

vvo commented Oct 16, 2017

@Haroenv Any way we can move forward with this PR? Is there anything we can do to help? Maybe the behaviour has changed now

@Haroenv
Copy link
Contributor Author

Haroenv commented Oct 16, 2017

Interesting, with newest RIS and React they show up twice, still multiple queries per refinement too...

https://codesandbox.io/s/oY1klpZYB

@Haroenv
Copy link
Contributor Author

Haroenv commented Dec 6, 2017

Whoa, there's no longer a duplicate query done, will look at the comments, then it's mergeable

@vvo
Copy link
Contributor

vvo commented Dec 7, 2017

Whoa, there's no longer a duplicate query done, will look at the comments, then it's mergeable

How come? :D

@Haroenv
Copy link
Contributor Author

Haroenv commented Jan 10, 2018

This is ready for review: https://deploy-preview-258--react-instantsearch.netlify.com/react-instantsearch/guide/refining_hits

The last section still needs to be verified based on #830

samouss pushed a commit that referenced this pull request Jan 11, 2018
…ems for deduplication (#830)

**Summary**

The currentRefinemements widget (and connector) will give exactly the same data as the widgets which are present in the layout. As seen in #258, when there are two refinements with the same attribute, this will give those items the same `key` and render with warnings.

**Result**

1. Give access to `index` and `id` (`${attributeName}`, `query`, ...) to `tranformItems` so users can deduplicate themselves
2. test how the deduplication can be done
@vvo
Copy link
Contributor

vvo commented Jan 16, 2018

Do we need anything else on this PR to move?

@Haroenv
Copy link
Contributor Author

Haroenv commented Jan 16, 2018

needs another review, implementation of that last deduplication in the CodeSandbox and a release of that

@samouss
Copy link
Collaborator

samouss commented Jan 16, 2018

I will review it today or tomorrow.


// if it's already refined, remove it from the refinements
const newValue = refinements[attributeName].slice();
newValue.splice(index, 1);
Copy link
Collaborator

@samouss samouss Jan 19, 2018

Choose a reason for hiding this comment

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

You can use a filter instead of a splice but it's matter of preference :)

Copy link
Collaborator

@samouss samouss left a comment

Choose a reason for hiding this comment

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

LGTM, but we need to test the last part after the release.

@samouss
Copy link
Collaborator

samouss commented Jan 19, 2018

Note that for this kind of behaviour it might be interesting to have something like a "controlled" widgets. For example by providing the currentRefinement to the widget. The prop became the source of truth instead of the searchState. In that case it will allow us to get rid of the part with the double check on defaultRefinement + currentRefinement since the "real" value will always be the currentRefinement.

In React the input element works a bit like that:

  • without a value the element is uncontrolled but you can still provide a defaultValue that will be used only for the first render (this is our behaviour)
  • with a value the element is controlled and you are responsible for all changes

@Haroenv
Copy link
Contributor Author

Haroenv commented Jan 19, 2018

Will that still allow changes from “upstream”?

@samouss
Copy link
Collaborator

samouss commented Jan 24, 2018

Sorry I didn’t see the question. What do you mean by that?

The version with the update on CurrentRefinements has been released so we can test the de duplication part.

render() {
return (
// the rest of the app
<CurrentRefinements transformData={deleteDuplicates} />
Copy link
Contributor Author

@Haroenv Haroenv Jan 24, 2018

Choose a reason for hiding this comment

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

should be transformItems

+ it doesn't work with more than 1 duplicate 🤔


```jsx
// ...
const deleteDuplicates = items =>
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 is wrong, should be:

export const deleteDuplicates = (items) =>
  items
    .map(item => {
      // make a temporary key for deduplication
      item.__dedupe = `${item.index}.${item.id}`;
      return item;
    })
    .filter(
      (obj, pos, arr) =>
        arr.map(item => item.__dedupe).indexOf(obj.__dedupe) === pos
    )
    .map(item => {
      // delete it afterwards
      delete item.__dedupe;
      return item;
    });

Copy link
Contributor Author

@Haroenv Haroenv Jan 25, 2018

Choose a reason for hiding this comment

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

export const deleteDuplicates = (items) =>
  items
    .filter(
      (obj, pos, arr) =>
        arr.map(item => `${item.index}.${item.id}`).indexOf(`${obj.index}.${obj.id}`) === pos
    )

two less iterations, but probably less performant due to string creation in hot loop (still O(n!))

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since the array is pretty small (less than 50 elements most of the time) I think the more understandable is the best 🙂

@samouss
Copy link
Collaborator

samouss commented Feb 13, 2018

@Haroenv I still have multiple queries but they won't be inevitable at some point since we play with the defaultRefinement. But for example when you click on the CurrentRefinements it triggers 4 requests. Maybe we should try remove some of them before merging this? I'm not sure that with the current state of the lib we could reduce the number of requests.

@samouss
Copy link
Collaborator

samouss commented Dec 10, 2018

We can't merge it right away because of the number of requests that are triggered. We keep the branch until we have a proper solution for the request issue.

@samouss samouss closed this Dec 10, 2018
@Haroenv Haroenv deleted the docs/refine-from-hits branch November 20, 2019 17:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error associating filters per each item
6 participants