Skip to content

Conversation

@francoischalifour
Copy link
Contributor

@francoischalifour francoischalifour commented Sep 27, 2018

@francoischalifour francoischalifour requested a review from a team September 27, 2018 08:38
@algobot
Copy link
Contributor

algobot commented Sep 27, 2018

Deploy preview for algolia-instantsearch ready!

Built with commit b7db199

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

{this.props.facetValues.map(this._generateFacetItem, this)}
<div
className={cx(this.props.cssClasses.root, {
[this.props.cssClasses.noRefinementRoot]: false,
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 is the condition for a refinement list to be refined?

Copy link
Contributor

Choose a reason for hiding this comment

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

when there's no possible values I think

this.props.showMore === true ? (
<Template
rootProps={{ onClick: this.props.toggleShowMore }}
rootTagName="button"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The template is now the button (see updated story).

Let me know if this is not what we want.

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 great 🔥 🔥

<svg width="16" height="16" viewBox="0 0 38 38" xmlns="http://www.w3.org/2000/svg" stroke="#444" class="ais-SearchBox-loadingIcon">
<g fill="none" fillRule="evenodd">
<g transform="translate(1 1)" strokeWidth="2">
<circle strokeOpacity=".5" cx="18" cy="18" r="18" />
Copy link
Contributor

Choose a reason for hiding this comment

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

also here and a few others :)

{this.props.facetValues.map(this._generateFacetItem, this)}
<div
className={cx(this.props.cssClasses.root, {
[this.props.cssClasses.noRefinementRoot]: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

when there's no possible values I think

return (
<div className={this.props.itemClassName} onClick={this.handleClick}>
<li className={this.props.cssClasses.item} onClick={this.handleClick}>
<Template
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 creates a wrapping div around the label from the template.

I can't think of an easy way to remove it because of the subItems that we want to render.

I don't think it's a big deal (we also have a wrapping div in the GeoSearch widget).

Copy link
Contributor

Choose a reason for hiding this comment

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

does Preact not have Fragments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, same issue with the GeoSearch widget (#3138 (comment)).

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems fine then :)

active: '<button>Show less</button>',
inactive: '<button>Show more</button>',
active: 'Show less',
inactive: 'Show more',
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought at some point we discussed about putting all the templates together rather than having multiple templates across options.

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 would make sense. Do you mean something like this?

{
  templates: {
    showMoreActive: '',
	showMoreInactive: '',
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes - same for searchable. But we can do it in another pass no worries.

| `ais-refinement-list--label` | `ais-RefinementList-label` |
| `ais-refinement-list--checkbox` | `ais-RefinementList-checkbox` |
| | `ais-RefinementList-labelText` |
| `ais-refinement-list--count` | `ais-RefinementList-count` |
Copy link
Contributor

Choose a reason for hiding this comment

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

Some classes are missing like active, showMore, noResults

: cx(
this.props.cssClasses.showMore,
this.props.cssClasses.disabledShowMore
);
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 use the object notation:

const showMoreButtonClassName = cx(this.props.cssClasses.showMore, {
  [this.props.cssClasses.disabledShowMore]:
    this.props.showMore === true && this.props.canToggleShowMore
});

);

const showMoreBtn =
this.props.showMore === true && this.props.canToggleShowMore ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we drop this condition this.props.canToggleShowMore ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

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's actually handled by the classname above, right?

From what I understand, if this.props.canToggleShowMore is false, we want to add the classname disabledShowMore.

Copy link
Contributor

Choose a reason for hiding this comment

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

With the v2 when you can't toggle the button is removed. React & Vue display the button with the disabled state. The first implementation was fine then.

const showMoreBtn =
this.props.showMore === true && this.props.canToggleShowMore ? (
const showMoreButton =
this.props.showMore === true ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the second part of the ternary is null we can use the && logical operator.

templateProps: {},
toggleRefinement: () => {},
};
const tree = renderer.create(<RefinementList {...props} />).toJSON();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we use React Test Renderer rather than Enzyme?

},
toggleRefinement: () => {},
};
const tree = renderer.create(<RefinementList {...props} />).toJSON();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we use React Test Renderer rather than Enzyme?

toggleRefinement: () => {},
createURL: () => {},
};
const tree = renderer.create(<RefinementList {...props} />).toJSON();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we use React Test Renderer rather than Enzyme?

* @property {string|string[]} [selectedItem] CSS class to add to each selected element.
* @property {string|string[]} [label] CSS class to add to each label element (when using the default template).
* @property {string|string[]} [checkbox] CSS class to add to each checkbox element (when using the default template).
* @property {string|string[]} [count] CSS class to add to each count element (when using the default template).
Copy link
Contributor

Choose a reason for hiding this comment

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

The count class is missing.

label: PropTypes.string,
checkbox: PropTypes.string,
labelText: PropTypes.string,
count: PropTypes.string,
Copy link
Contributor

Choose a reason for hiding this comment

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

For this I was more talking about why do we have switched from string to object? Since there is only one element rendered in the component it makes sense to have className rather than cssClasses.

@francoischalifour francoischalifour force-pushed the feat/3.0-refinement-list branch from b3ef692 to f0aa328 Compare October 3, 2018 17:24
@francoischalifour francoischalifour force-pushed the feat/3.0-refinement-list branch from 194292d to b7db199 Compare October 3, 2018 18:18
@francoischalifour francoischalifour merged commit 11c5580 into feat/3.0 Oct 3, 2018
@francoischalifour francoischalifour deleted the feat/3.0-refinement-list branch October 3, 2018 20:58
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