Skip to content

Conversation

@francoischalifour
Copy link
Contributor

@francoischalifour francoischalifour commented Oct 11, 2018

@algobot
Copy link
Contributor

algobot commented Oct 11, 2018

Deploy preview for algolia-instantsearch ready!

Built with commit e93e312

https://deploy-preview-3190--algolia-instantsearch.netlify.com

refinement.name;

return (
<li className={this.props.cssClasses.item} key={key}>
Copy link
Contributor Author

@francoischalifour francoischalifour Oct 11, 2018

Choose a reason for hiding this comment

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

I used a conditional template because we're using React props in the default template.

Note that there's no way for the user to use a custom template and to remove a filter (because the this.props.clearRefinementClicks is not accessible).

Copy link
Contributor

Choose a reason for hiding this comment

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

wait, if a custom template can't remove a filter, it's broken, right?

return {
...item,
attributeName,
computedLabel,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we simply go for label?

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.

I don't understand what you mean with the remark about double template and fragments. The output DOM seems fine to me.

What do you mean about the custom templates not being able to clear though?

search.addWidget(
instantsearch.widgets.currentRefinedValues({
instantsearch.widgets.currentRefinements({
container: '#current-refined-values',
Copy link
Contributor

Choose a reason for hiding this comment

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

change the id too maybe?

import Template from '../Template.js';
import { isSpecialClick } from '../../lib/utils.js';

function handleClick(cb) {
Copy link
Contributor

Choose a reason for hiding this comment

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

irrelevant for this PR, but seems like handleClick & isSpecialClick are always used together. It should be a single function "avoidSpecialClick" instead IMO

refinement.name;

return (
<li className={this.props.cssClasses.item} key={key}>
Copy link
Contributor

Choose a reason for hiding this comment

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

wait, if a custom template can't remove a filter, it's broken, right?

import React, { Component } from 'preact-compat';
import PropTypes from 'prop-types';
import isEqual from 'lodash/isEqual';
import upperFirst from 'lodash/upperFirst';
Copy link
Contributor

Choose a reason for hiding this comment

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

this is actually a quite big package, and I think we only use it here. We can rewrite it (later?) to a smaller version that doesn't deal with so many edge cases as the lodash version

return 1;
}

function normalizeRefinements(refinements, helper) {
Copy link
Contributor Author

@francoischalifour francoischalifour Oct 18, 2018

Choose a reason for hiding this comment

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

Refinements attributes and items are sorted alphabetically. Is that what we want?

Copy link
Contributor

Choose a reason for hiding this comment

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

As I said, sorting is something we never specified so to me alpha makes more sense than the complicated logic it had before, but no sorting at all is also an option for me.

| `attributes` | `includedAttributes` |
| | `excludedAttributes` |
| `onlyListedAttributes` | use `includedAttributes` |
| `clearsQuery` | use `includedAttributes` |
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be use excludedAttributes.

Worth adding an example that show how the previous implementation was working vs the current one.

attribute: PropTypes.string.isRequired,
refine: PropTypes.func.isRequired,
items: PropTypes.arrayOf(
PropTypes.shape({
Copy link
Contributor

Choose a reason for hiding this comment

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

We can create variable for those shapes, it will be easier to read.

`${[attribute, type, value, operator]
.map(key => key)
.filter(Boolean)
.join(':')}#${index + 1}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really have to provide the index? We can assume that for each refinement the 4-tuple is a unique identifier? It doesn't make sense to have two refinement with the exact same information. Did you have any issue without the index?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It did because a refinement had a duplicate attribute in a test. Fixing it!


class CurrentRefinements extends Component {
shouldComponentUpdate(nextProps) {
return !isEqual(this.props.refinements, nextProps.refinements);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really have to implement this hook? Did you notice any performance bottleneck?

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 was in the previous implementation. We can remove it for now.

value,
label,
...(item.operator && { operator: item.operator }),
...(item.count && { count: item.count }),
Copy link
Contributor

Choose a reason for hiding this comment

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

With a count of 0 the object won't be spread.

label,
...(item.operator && { operator: item.operator }),
...(item.count && { count: item.count }),
...(item.exhaustive && { exhaustive: item.exhaustive }),
Copy link
Contributor

Choose a reason for hiding this comment

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

With a non exhaustive count the object won't be spread.

refinementItem.value
);
case 'hierarchical':
return state.clearRefinements(refinementItem.attribute);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we using clearRefinements rather than removeHierarchicalFacetRefinement? The latter avoid to drop other refinements in case there are some on the same attribute.

className={this.props.cssClasses.category}
>
<span className={this.props.cssClasses.categoryLabel}>
{item.attribute === 'query' ? <q>{item.label}</q> : item.label}
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't provide template for this widget?

Copy link
Contributor

Choose a reason for hiding this comment

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

We discussed it, and the template doesn't provide anything transformItems can't

It becomes especially tricky with the nested form; where does the template go? in the inner bit means you can't customise the main label 🤔

Copy link
Contributor Author

@francoischalifour francoischalifour Oct 24, 2018

Choose a reason for hiding this comment

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

Right, the template doesn't really make sense here. If the template is the whole refinement attribute, the refine function won't be easily usable. If the template is only the label, it can be edited more easily with transformItems.

| `clearsQuery` | use `excludedAttributes` |
| `clearAll` | use the `clearRefinements` widget |

- `clearsQuery` can replaced by `excludedAtributes: ['query']`.
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 it should be excludedAtributes: [].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, thanks!

* @typedef {Object} CurrentRefinementsRenderingOptions
* @property {function(item)} refine Clears a single refinement
* @property {function(item): string} createURL Creates an individual URL where a single refinement is cleared
* @property {CurrentRefinement[]} refinements All the current refinements
Copy link
Contributor

Choose a reason for hiding this comment

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

Yep the swap seems a good solution!

@Haroenv
Copy link
Contributor

Haroenv commented Oct 26, 2018

Copy link
Contributor

@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.

@samouss
Copy link
Contributor

samouss commented Oct 26, 2018

The fact the the values are sorted in the list feels a bit weird. Are we sure that we want to sort them? To me it feels more natural to append them last refined - last item in the list.

@Haroenv
Copy link
Contributor

Haroenv commented Oct 27, 2018

@samouss apparently they were always sorted in InstantSearch.js, but originally more complex than this. Either sorting the same as in the widget or by addition makes sense to me. As long as things don’t completely shuffle around if you change something it’s good for me

@francoischalifour
Copy link
Contributor Author

francoischalifour commented Oct 28, 2018

@samouss I did a little bit of research on existing work online but couldn't find any interesting work on this. I can remove the sorting and see if the behavior seems intuitive.

@Haroenv What is the reason for these weird custom connectors + jQuery in the first place? Could they all be replaced by the new Algolia documentation we're working on?

I reckon they're hard to maintain and not very insightful.

@francoischalifour francoischalifour merged commit a70917d into feat/3.0 Oct 29, 2018
@francoischalifour francoischalifour deleted the feat/3.0-current-refinements branch October 29, 2018 12:13
francoischalifour added a commit that referenced this pull request Oct 29, 2018
Following #3190 (comment), we decided to remove the connectors + jQuery stories for maintainability and usefulness purpose.

These stories will get replaced by the new Algolia documentation.
francoischalifour added a commit that referenced this pull request Dec 10, 2018
<a name=3.0.0-beta.0></a>
# [3.0.0-beta.0](v2.10.3...v3.0.0-beta.0) (2018-12-10)

### Bug Fixes

* **api:** remove transformData ([#3241](#3241)) ([5232936](5232936))
* **breadcrumb:** fix story with transformed label ([5a39312](5a39312))
* **breadcrumb:** rename item's name to label ([#3273](#3273)) ([0189cb4](0189cb4))
* **breadcrumb:** rename noRefinement to noRefinementRoot ([#3272](#3272)) ([21a5c61](21a5c61))
* **community:** fix search config ([#3142](#3142)) ([2b41982](2b41982))
* **connectRangeSlider:** remove deprecated connector ([#3189](#3189)) ([096ebeb](096ebeb))
* **current-refinements:** remove alphabetic sorting ([#3249](#3249)) ([9914f87](9914f87)), closes [/github.com/algolia/instantsearch.js/blob/011d47909894837f2c2da5088bc81803135d201b/src/lib/utils.js#L249](https://github.com//github.com/algolia/instantsearch.js/blob/011d47909894837f2c2da5088bc81803135d201b/src/lib/utils.js/issues/L249)
* **getRefinements:** provide attributeName for type: query ([6006fe1](6006fe1)), closes [#3205](#3205)
* **highlight:** avoid to ignore highlightedTagName ([#3323](#3323)) ([9871829](9871829))
* **highlight:** HighLight -> Highlight ([#3324](#3324)) ([5b4600d](5b4600d))
* **hits:** transform items after escaping ([#3251](#3251)) ([c46b82a](c46b82a)), closes [#3250](#3250)
* **infiniteHits:** move  option to templates ([#3300](#3300)) ([1828d66](1828d66))
* **InfiniteHits:** set the correct class for the last page ([#3232](#3232)) ([f604835](f604835))
* **lint:** remove unused import ([a9ec14c](a9ec14c))
* **pagination:** rename  to  ([#3275](#3275)) ([336945b](336945b))
* **range-input:** fix button classname ([#3234](#3234)) ([56695e1](56695e1)), closes [algolia/instantsearch-specs#92](algolia/instantsearch-specs#92)
* **range-input:** remove templates ([#3128](#3128)) ([94a1ce5](94a1ce5))
* **rangeInput:** convert labels to templates ([#3312](#3312)) ([cdf91e8](cdf91e8))
* **rangeSlider:** fix CSS classes ([#3316](#3316)) ([56a5255](56a5255))
* **refinement-list:** remove  in story ([0bf8db1](0bf8db1))
* **routing:** enforce RoutingManager is the last widget ([#3149](#3149)) ([1e86b2e](1e86b2e)), closes [#3148](#3148)
* **searchbox:** fix  and  templates ([#3313](#3313)) ([4e13122](4e13122))
* **tests:** add react-test-renderer ([176494b](176494b))
* **toggleRefinement:** provide  to templates ([#3303](#3303)) ([f515b62](f515b62))

### Features

* **3.0:** remove named exports on widgets ([#3129](#3129)) ([e718ea3](e718ea3))
* **breadcrumb:** implement InstantSearch.css ([#3115](#3115)) ([84d9f18](84d9f18))
* **clearRefinements:** implement InstantSearch.css ([#3308](#3308)) ([d98ecaf](d98ecaf)), closes [#3299](#3299)
* **clearRefinements:** implement is.css ([#3114](#3114)) ([11cdc14](11cdc14))
* **current-refinements:** implement InstantSearch.css ([#3190](#3190)) ([a70917d](a70917d))
* **GeoSearch:** implement InstantSearch.css ([#3138](#3138)) ([1867d30](1867d30))
* **hierarchical-menu:** implement InstantSearch.css ([#3182](#3182)) ([be0890d](be0890d))
* **hierarchical-menu:** implement show more feature ([#3151](#3151)) ([f54fccd](f54fccd))
* **hierarchicalMenu:** merge showMore templates ([#3318](#3318)) ([0059251](0059251))
* **highlight:** export highlight function ([#3137](#3137)) ([d4b6fb1](d4b6fb1))
* **highlight:** implement InstantSearch.css ([#3132](#3132)) ([260a0b8](260a0b8))
* **hits:** implement InstantSearch.css ([#3096](#3096)) ([b3cc413](b3cc413))
* **hits-per-page:** implement InstantSearch.css ([#3125](#3125)) ([49e7096](49e7096))
* **menu:** implement InstantSearch.css ([#3181](#3181)) ([a274ab7](a274ab7))
* **menuSelect:** implement is.css ([#3109](#3109)) ([43e654a](43e654a))
* **numeric-menu:** implement InstantSearch.css ([#3162](#3162)) ([f5358f4](f5358f4))
* **numericSelector:** remove widget ([#3183](#3183)) ([e9063c0](e9063c0))
* **pagination:** implement InstantSearch.css ([#3119](#3119)) ([f3c3343](f3c3343))
* **panel:** add Panel widget ([#3253](#3253)) ([82e19fc](82e19fc))
* **poweredBy:** implement InstantSearch.css ([#3164](#3164)) ([bcc18a0](bcc18a0))
* **poweredBy:** update logo ([#3256](#3256)) ([838abec](838abec))
* **price-ranges:** implement InstantSearch.css ([#3124](#3124)) ([335339b](335339b))
* **range-input:** implement InstantSearch.css ([#3098](#3098)) ([ee6bc7e](ee6bc7e))
* **range-slider:** implement InstantSearch.css ([#3126](#3126)) ([b9b8d31](b9b8d31))
* **rating-menu:** implement InstantSearch.css ([#3161](#3161)) ([d039e11](d039e11))
* **ratingMenu:** merge labels and templates ([#3317](#3317)) ([505a2e7](505a2e7))
* **refinement-list:** implement InstantSearch.css ([#3152](#3152)) ([11c5580](11c5580))
* **refinement-list:** implement InstantSearch.css (2) ([#3179](#3179)) ([0365641](0365641))
* **refinement-list:** implement InstantSearch.css to searchbox ([#3263](#3263)) ([ad905c7](ad905c7))
* **search-client:** use search client ([#3133](#3133)) ([8e70a3e](8e70a3e))
* **searchbox:** implement InstantSearch.css ([#3127](#3127)) ([c68c1fe](c68c1fe))
* **snippet:** implement InstantSearch.css ([#3134](#3134)) ([fa56657](fa56657))
* **sort-by:** implement InstantSearch.css ([#3120](#3120)) ([5f21723](5f21723))
* **sortBy:** rename item  to  ([#3230](#3230)) ([9e24a68](9e24a68))
* **stats:** implement InstantSearch.css ([#3097](#3097)) ([63a688e](63a688e))
* **stories:** add default CurrentRefinements story ([#3252](#3252)) ([45a8fd5](45a8fd5))
* **suit:** Default component names to empty object ([0b26356](0b26356))
* **suit-helper:** provide a helper to create suit css classnames ([f142496](f142496))
* **toggleRefinement:** implement InstantSearch.css ([#3135](#3135)) ([d67a437](d67a437))
* compress templates ([#3176](#3176)) ([54f2f77](54f2f77)), closes [#3095](#3095)
* **widgets:** use warn utils ([#3175](#3175)) ([3164b06](3164b06))
francoischalifour added a commit that referenced this pull request Dec 14, 2018
<a name=3.0.0-beta.1></a>
# [3.0.0-beta.1](v2.10.3...v3.0.0-beta.1) (2018-12-14)

### Bug Fixes

* **api:** remove transformData ([#3241](#3241)) ([5232936](5232936))
* **breadcrumb:** fix story with transformed label ([5a39312](5a39312))
* **breadcrumb:** rename item's name to label ([#3273](#3273)) ([0189cb4](0189cb4))
* **breadcrumb:** rename noRefinement to noRefinementRoot ([#3272](#3272)) ([21a5c61](21a5c61))
* **community:** fix search config ([#3142](#3142)) ([2b41982](2b41982))
* **configure:** make lifecycle functions optional ([#3339](#3339)) ([2b88cfa](2b88cfa))
* **connectRangeSlider:** remove deprecated connector ([#3189](#3189)) ([096ebeb](096ebeb))
* **current-refinements:** remove alphabetic sorting ([#3249](#3249)) ([9914f87](9914f87)), closes [/github.com/algolia/instantsearch.js/blob/011d47909894837f2c2da5088bc81803135d201b/src/lib/utils.js#L249](https://github.com//github.com/algolia/instantsearch.js/blob/011d47909894837f2c2da5088bc81803135d201b/src/lib/utils.js/issues/L249)
* **getRefinements:** provide attributeName for type: query ([6006fe1](6006fe1)), closes [#3205](#3205)
* **highlight:** avoid to ignore highlightedTagName ([#3323](#3323)) ([9871829](9871829))
* **highlight:** HighLight -> Highlight ([#3324](#3324)) ([5b4600d](5b4600d))
* **hits:** transform items after escaping ([#3251](#3251)) ([c46b82a](c46b82a)), closes [#3250](#3250)
* **infiniteHits:** move  option to templates ([#3300](#3300)) ([1828d66](1828d66))
* **InfiniteHits:** set the correct class for the last page ([#3232](#3232)) ([f604835](f604835))
* **lint:** remove unused import ([a9ec14c](a9ec14c))
* **pagination:** rename  to  ([#3275](#3275)) ([336945b](336945b))
* **poweredBy:** export connectPoweredBy connector ([#3331](#3331)) ([7d48c46](7d48c46))
* **poweredBy:** fix CSS classes ([#3332](#3332)) ([abc9b82](abc9b82))
* **range-input:** fix button classname ([#3234](#3234)) ([56695e1](56695e1)), closes [algolia/instantsearch-specs#92](algolia/instantsearch-specs#92)
* **range-input:** remove templates ([#3128](#3128)) ([94a1ce5](94a1ce5))
* **rangeInput:** convert labels to templates ([#3312](#3312)) ([cdf91e8](cdf91e8))
* **rangeSlider:** fix CSS classes ([#3316](#3316)) ([56a5255](56a5255))
* **refinement-list:** remove  in story ([0bf8db1](0bf8db1))
* **routing:** enforce RoutingManager is the last widget ([#3149](#3149)) ([1e86b2e](1e86b2e)), closes [#3148](#3148)
* **searchbox:** fix  and  templates ([#3313](#3313)) ([4e13122](4e13122))
* **tests:** add react-test-renderer ([176494b](176494b))
* **toggleRefinement:** provide  to templates ([#3303](#3303)) ([f515b62](f515b62))

### Features

* **3.0:** remove named exports on widgets ([#3129](#3129)) ([e718ea3](e718ea3))
* **breadcrumb:** implement InstantSearch.css ([#3115](#3115)) ([84d9f18](84d9f18))
* **bundle:** update bundle strategy ([#3260](#3260)) ([a7dab81](a7dab81))
* **clearRefinements:** implement InstantSearch.css ([#3308](#3308)) ([d98ecaf](d98ecaf)), closes [#3299](#3299)
* **clearRefinements:** implement is.css ([#3114](#3114)) ([11cdc14](11cdc14))
* **current-refinements:** implement InstantSearch.css ([#3190](#3190)) ([a70917d](a70917d))
* **GeoSearch:** implement InstantSearch.css ([#3138](#3138)) ([1867d30](1867d30))
* **hierarchical-menu:** implement InstantSearch.css ([#3182](#3182)) ([be0890d](be0890d))
* **hierarchical-menu:** implement show more feature ([#3151](#3151)) ([f54fccd](f54fccd))
* **hierarchicalMenu:** merge showMore templates ([#3318](#3318)) ([0059251](0059251))
* **highlight:** export highlight function ([#3137](#3137)) ([d4b6fb1](d4b6fb1))
* **highlight:** implement InstantSearch.css ([#3132](#3132)) ([260a0b8](260a0b8))
* **hits:** implement InstantSearch.css ([#3096](#3096)) ([b3cc413](b3cc413))
* **hits-per-page:** implement InstantSearch.css ([#3125](#3125)) ([49e7096](49e7096))
* **infiniteHits:** rename showMore template to showMoreText ([#3330](#3330)) ([babad39](babad39))
* **menu:** implement InstantSearch.css ([#3181](#3181)) ([a274ab7](a274ab7))
* **menu:** merge showMore templates ([#3328](#3328)) ([73a450b](73a450b))
* **menuSelect:** implement is.css ([#3109](#3109)) ([43e654a](43e654a))
* **numeric-menu:** implement InstantSearch.css ([#3162](#3162)) ([f5358f4](f5358f4))
* **numericSelector:** remove widget ([#3183](#3183)) ([e9063c0](e9063c0))
* **pagination:** implement InstantSearch.css ([#3119](#3119)) ([f3c3343](f3c3343))
* **pagination:** rename labels to templates ([#3333](#3333)) ([9f24098](9f24098))
* **panel:** add Panel widget ([#3253](#3253)) ([82e19fc](82e19fc))
* **poweredBy:** disable setting URL from widget ([#3334](#3334)) ([a5ff6af](a5ff6af))
* **poweredBy:** implement InstantSearch.css ([#3164](#3164)) ([bcc18a0](bcc18a0))
* **poweredBy:** update logo ([#3256](#3256)) ([838abec](838abec))
* **price-ranges:** implement InstantSearch.css ([#3124](#3124)) ([335339b](335339b))
* **range-input:** implement InstantSearch.css ([#3098](#3098)) ([ee6bc7e](ee6bc7e))
* **range-slider:** implement InstantSearch.css ([#3126](#3126)) ([b9b8d31](b9b8d31))
* **rating-menu:** implement InstantSearch.css ([#3161](#3161)) ([d039e11](d039e11))
* **ratingMenu:** merge labels and templates ([#3317](#3317)) ([505a2e7](505a2e7))
* **refinement-list:** implement InstantSearch.css ([#3152](#3152)) ([11c5580](11c5580))
* **refinement-list:** implement InstantSearch.css (2) ([#3179](#3179)) ([0365641](0365641))
* **refinement-list:** implement InstantSearch.css to searchbox ([#3263](#3263)) ([ad905c7](ad905c7))
* **refinementList:** merge showMore templates ([#3329](#3329)) ([9b6a9c4](9b6a9c4))
* **search-client:** use search client ([#3133](#3133)) ([8e70a3e](8e70a3e))
* **searchbox:** implement InstantSearch.css ([#3127](#3127)) ([c68c1fe](c68c1fe))
* **snippet:** implement InstantSearch.css ([#3134](#3134)) ([fa56657](fa56657))
* **sort-by:** implement InstantSearch.css ([#3120](#3120)) ([5f21723](5f21723))
* **sortBy:** rename item  to  ([#3230](#3230)) ([9e24a68](9e24a68))
* **stats:** implement InstantSearch.css ([#3097](#3097)) ([63a688e](63a688e))
* **stories:** add default CurrentRefinements story ([#3252](#3252)) ([45a8fd5](45a8fd5))
* **suit:** Default component names to empty object ([0b26356](0b26356))
* **suit-helper:** provide a helper to create suit css classnames ([f142496](f142496))
* **toggleRefinement:** implement InstantSearch.css ([#3135](#3135)) ([d67a437](d67a437))
* **widgets:** use warn utils ([#3175](#3175)) ([3164b06](3164b06))
* compress templates ([#3176](#3176)) ([54f2f77](54f2f77)), closes [#3095](#3095)
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.

5 participants