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/instantsearch.js/unify connector api #2107

Merged
merged 65 commits into from Apr 24, 2017

Conversation

Projects
None yet
3 participants
@iam4x
Copy link
Contributor

iam4x commented Apr 10, 2017

WIP: this list may change in the near future

  • connectMenu

    • easier implementation of showMore
    • remove currentRefinement which is not used
  • connectHits

    • remove hitsPerPage config option
  • connectClearAll

    • provide clearAll fn on first render
    • provide same instance/reference to clearAll fn
    • add {clearsQuery: boolean} opt-in
  • connectCurrentRefinedValues

    • Move displayOperator logic into connector, compute items[i].name for the user
    • Use clearRefinement(item) and createURL(item) instead of clearRefinementURLs[] and clearRefinementClicks[]
    • add {clearsQuery: boolean} opt-in
  • connectHitsPerPageSelector

    • Rename options[] to items[]
    • Add isRefined to the items[]
    • Rename connectHitsPerPageSelector to connectHitsPerPage
  • connectPriceRanges

    • Make refine() taking named arguments instead of two arguments => refine({ from: ?number, to: ?number })
  • connectToggle

    • fix createURL fn which is not returning a string
    • fix root level count when there's no off value
    • Make refine() taking one argument which is the value of the toggle
  • Other

    • Remove currentRefinement from connectRefinementList, connectNumericRefinementList, connectPriceRanges, connectStarRating
    • Investigate why pagination widget is not working in demo
    • Check that refine() and createURL() takes item.value instead of a different value everywhere
      • connectHierarchicalMenu
      • connectMenu
      • connectNumericRefinementList
      • connectRefinementList
      • connectStarRating

iam4x and others added some commits Apr 10, 2017

chore(connectClearAll): vanilla example can now be simpler
same function reference, so it can be bound once
fix(connectSearchBox): rename search to refine
It's more similar to the other refinements

note: this is a breaking change
feat(connectHitsPerPage): replace console.log by console.warn
It's shown as a warning, and in yellow. Supported from IE8

iam4x and others added some commits Apr 13, 2017

fix(Selector): invalid `propTypes` definitions
* `currentValue` can be null/empty
* `options[i].value` can be null/empty

For instance `hit-per-page-selector` have default value of null
fix(<RefinementListItem />): incorrect propTypes for `facetValue.value`
For instance, when it's using `toggle` widget there's no value to refine

@bobylito bobylito added this to Unknown status in Triage Apr 18, 2017

@bobylito bobylito removed this from Unknown status in Triage Apr 18, 2017

@iam4x iam4x referenced this pull request Apr 18, 2017

Closed

Feat/new range slider #2112

4 of 4 tasks complete
@Haroenv
Copy link
Member

Haroenv left a comment

Small questions, I think this is good to land otherwise

clearRefinementURLs,
clearRefinementClicks,
createURL,
clearRefinement,

This comment has been minimized.

@Haroenv

Haroenv Apr 19, 2017

Member

Why is this not called refine

This comment has been minimized.

@iam4x

iam4x Apr 19, 2017

Author Contributor

I don't think so, it's the opposite of refine() action?

This comment has been minimized.

@Haroenv

Haroenv Apr 19, 2017

Member

yes, maybe this just is the best name.

This comment has been minimized.

@iam4x

iam4x Apr 19, 2017

Author Contributor

Go for refine() see: #2107 (comment)

@@ -46,8 +47,8 @@ Full documentation available at https://community.algolia.com/instantsearch.js/c
* @property {function} clearAllClick function to trigger the clear of all the currently refined values
* @property {string} clearAllPosition position of the 'clear all' button
* @property {function} clearAllURL url which leads to a state where all the refinements have been cleared
* @property {function[]} clearRefinementClicks individual clearing function per refinement
* @property {string[]} clearRefinementURLs individual url where a single refinement is cleared
* @property {function(item)} clearRefinement clearing function for a refinement

This comment has been minimized.

@Haroenv

Haroenv Apr 19, 2017

Member

here too, should be refine IMO

@@ -65,6 +66,7 @@ Full documentation available at https://community.algolia.com/instantsearch.js/c
* @typedef {Object} CurrentRefinedValuesWidgetOptions
* @property {CurrentRefinedValuesAttributes[]} attributes specification for the display of refinements per attribute
* @property {boolean} onlyListedAttributes limit the displayed refinement to the list specified
* @property {boolean} clearsQuery also clears the active search query

This comment has been minimized.

@Haroenv

Haroenv Apr 19, 2017

Member

say that the default is false

This comment has been minimized.

@iam4x

iam4x Apr 19, 2017

Author Contributor

👍

This comment has been minimized.

@iam4x

iam4x Apr 19, 2017

Author Contributor

Should we do it that way:

* @property {boolean = false} clearsQuery also clears the active search query

cc @bobylito ?

This comment has been minimized.

@bobylito

bobylito Apr 19, 2017

Contributor

you should write [clearsQuery=false]

function computeLabel(value) {
if (value.hasOwnProperty('operator') && typeof value.operator === 'string') {
let displayedOperator = value.operator;
if (value.operator === '>=') displayedOperator = '';

This comment has been minimized.

@Haroenv

Haroenv Apr 19, 2017

Member

maybe using a switch here makes more sense in case there would be more supported operators?

This comment has been minimized.

@bobylito

bobylito Apr 19, 2017

Contributor

We can figure this out later when we have more operators.

@@ -214,3 +218,14 @@ function clearRefinementFromState(state, refinement) {
function clearRefinement(helper, refinement) {
helper.setState(clearRefinementFromState(helper.state, refinement)).search();
}

function computeLabel(value) {

This comment has been minimized.

@Haroenv

Haroenv Apr 19, 2017

Member

Can this somehow be pluggable?

This comment has been minimized.

@iam4x

iam4x Apr 19, 2017

Author Contributor

Do you mean to externalize somewhere else this logic or a way to override this function through connector options?

This comment has been minimized.

@bobylito

bobylito Apr 19, 2017

Contributor

What do you mean?

This comment has been minimized.

@Haroenv

Haroenv Apr 19, 2017

Member

not sure if it makes sense, but those symbols might be different in other alphabets etc, so this should be configurable (after we looked some up about it)

also some people might want to have this written in full "less than x more than y".

Might be an edge case though

This comment has been minimized.

@iam4x

iam4x Apr 19, 2017

Author Contributor

I think it should come after if it's asked.

This comment has been minimized.

@bobylito

bobylito Apr 19, 2017

Contributor

Agree that we can wait for the need to be expressed.

@@ -115,6 +119,7 @@ currentRefinedValues({
* @param {string} [$0.cssClasses.footer] CSS classes added to the footer element
* @param {object|boolean} [$0.collapsible=false] Hide the widget body and footer when clicking on header
* @param {boolean} [$0.collapsible.collapsed] Initial collapsed state of a collapsible widget
* @param {boolean} [$0.clearsQuery] also clears the active search query

This comment has been minimized.

@Haroenv

Haroenv Apr 19, 2017

Member

maybe give the $0 a name so that it gets displayed properly in the docs

This comment has been minimized.

@Haroenv

Haroenv Apr 19, 2017

Member

say that the default is false in jsDoc

This comment has been minimized.

@bobylito

bobylito Apr 19, 2017

Contributor

You can't give it a name otherwise, it sorts of breaks the documentation.

This comment has been minimized.

@Haroenv

Haroenv Apr 19, 2017

Member

I thought we said something about giving it a typedef just for that?

This comment has been minimized.

@bobylito

bobylito Apr 19, 2017

Contributor

That we will go over when going through the doc.

@@ -126,7 +127,7 @@ exports[`hierarchicalMenu() render sets facetValues to empty array when no resul
"templates": Object {
"footer": "",
"header": "",
"item": "<a class=\\"{{cssClasses.link}}\\" href=\\"{{url}}\\">{{name}} <span class=\\"{{cssClasses.count}}\\">{{#helpers.formatNumber}}{{count}}{{/helpers.formatNumber}}</span></a>",
"item": "<a class=\\"{{cssClasses.link}}\\" href=\\"{{url}}\\">{{label}} <span class=\\"{{cssClasses.count}}\\">{{#helpers.formatNumber}}{{count}}{{/helpers.formatNumber}}</span></a>",

This comment has been minimized.

@Haroenv

Haroenv Apr 19, 2017

Member

Should we deprecate the templates now or in another major?

This comment has been minimized.

@bobylito

bobylito Apr 19, 2017

Contributor

The deprecation of templates is not final. Let's not rush it.

refine,
hasNoResults,
}, isFirstRendering) => {
if (isFirstRendering) return;

const {value: currentValue} = items.find(({isRefined}) => isRefined) || {};

This comment has been minimized.

@Haroenv

Haroenv Apr 19, 2017

Member

this should be filter instead of find, right?

Also maybe just removed?

This comment has been minimized.

@bobylito

bobylito Apr 19, 2017

Contributor

HitsPerPage can only have one selected value so find is fine.

@@ -7,6 +7,8 @@ import numericRefinementList from '../numeric-refinement-list.js';
import RefinementList from '../../../components/RefinementList/RefinementList.js';
expect.extend(expectJSX);

const encodeValue = (start, end) => window.encodeURI(JSON.stringify({start, end}));

This comment has been minimized.

@Haroenv

Haroenv Apr 19, 2017

Member

same as I said earlier

This comment has been minimized.

@iam4x

iam4x Apr 19, 2017

Author Contributor

Explained into #2107 (comment)

@@ -66,7 +66,7 @@ const renderer = ({
showMore={showMoreConfig !== null}
templateProps={renderState.templateProps}
toggleRefinement={refine}
searchFacetValues={searchForFacetValues && searchForItems}
searchFacetValues={searchForFacetValues ? searchForItems : () => {}}

This comment has been minimized.

@Haroenv

Haroenv Apr 19, 2017

Member

Why not undefined as default value?

This comment has been minimized.

@Haroenv

Haroenv Apr 19, 2017

Member

Since people shouldn't call this in that case, right?

This comment has been minimized.

@bobylito

bobylito Apr 19, 2017

Contributor

It should be undefined if it's not activated. In the current implementation of the refinement list component, the contract is that is there is a function for searchFacetValues, it means that the component should render a search box. If not, no search box.

https://github.com/algolia/instantsearch.js/blob/feat/instantsearch.js/v2/src/components/RefinementList/RefinementList.js#L166-L171

This comment has been minimized.

@iam4x

iam4x Apr 19, 2017

Author Contributor

Because RefinementList is awaiting a PropTypes.function for searchFacetValues should we change it to oneOfType([ ... ]) ?

This comment has been minimized.

@Haroenv

Haroenv Apr 19, 2017

Member

maybe there's a way to just not set the prop?

Can the spread operator help with that or is it overkill

This comment has been minimized.

@iam4x

iam4x Apr 19, 2017

Author Contributor

I find it overkill and not beautiful to use the spread operator to achieve this, I can do either:

searchFacetValues={searchForFacetValues ? searchForItems : () => {}}

or

searchFacetValues={searchForFacetValues ? searchForItems : undefined}

This comment has been minimized.

@bobylito

bobylito Apr 19, 2017

Contributor

I think the least impactful is not to change the RefinementList, update the propType and use undefined. If only we had option types 🤓

@bobylito
Copy link
Contributor

bobylito left a comment

A few changes and some questions :) But overall awesome, that's a big task 🚀


markup
.on('click', e => hasRefinements ? refine() : e.preventDefault());

This comment has been minimized.

@bobylito

bobylito Apr 19, 2017

Contributor

User shouldn't have to be smart here :)

markup.on('click', refine);

should do the trick (and maybe we need to do this check somewhere in our stack)

.map(value => {
const {name, count} = value;
const {labelWithOperator, count} = value;
const name = labelWithOperator || value.name;

This comment has been minimized.

@bobylito

bobylito Apr 19, 2017

Contributor

Isn't there always a value for labelWithOperator? Maybe it should?

This comment has been minimized.

@iam4x

iam4x Apr 24, 2017

Author Contributor

Doesn't have always an operator, renamed labelWithOperator to computedLabel which defaults to value.name

see 2e77e1d

const inputValue = inputNode.val();
if (inputValue !== query) search(inputValue);
if (inputValue !== query) refine(inputValue);
@@ -65,6 +66,7 @@ Full documentation available at https://community.algolia.com/instantsearch.js/c
* @typedef {Object} CurrentRefinedValuesWidgetOptions
* @property {CurrentRefinedValuesAttributes[]} attributes specification for the display of refinements per attribute
* @property {boolean} onlyListedAttributes limit the displayed refinement to the list specified
* @property {boolean} clearsQuery also clears the active search query

This comment has been minimized.

@bobylito

bobylito Apr 19, 2017

Contributor

you should write [clearsQuery=false]

@@ -214,3 +218,14 @@ function clearRefinementFromState(state, refinement) {
function clearRefinement(helper, refinement) {
helper.setState(clearRefinementFromState(helper.state, refinement)).search();
}

function computeLabel(value) {

This comment has been minimized.

@bobylito

bobylito Apr 19, 2017

Contributor

Agree that we can wait for the need to be expressed.

@@ -145,14 +145,14 @@ describe('connectHitsPerPageSelector', () => {
});

const secondRenderingOptions = rendering.lastCall.args[0];
expect(secondRenderingOptions.currentRefinement).toBe(3);
expect(secondRenderingOptions.items.find(({isRefined}) => isRefined).value).toBe(3);

This comment has been minimized.

@bobylito

bobylito Apr 19, 2017

Contributor

I would match the whole structure instead on relying on a programmatic way.

expectedResults1[4].isRefined = true;

const renderingParameters1 = rendering.lastCall.args[0];
expect(renderingParameters1.items).toEqual(expectedResults1);
});

it('provides the correct `currentRefinement` value', () => {

This comment has been minimized.

@bobylito

bobylito Apr 19, 2017

Contributor

Do we test that there is a value that has isRefined after the use of refine?

@@ -115,6 +119,7 @@ currentRefinedValues({
* @param {string} [$0.cssClasses.footer] CSS classes added to the footer element
* @param {object|boolean} [$0.collapsible=false] Hide the widget body and footer when clicking on header
* @param {boolean} [$0.collapsible.collapsed] Initial collapsed state of a collapsible widget
* @param {boolean} [$0.clearsQuery] also clears the active search query

This comment has been minimized.

@bobylito

bobylito Apr 19, 2017

Contributor

That we will go over when going through the doc.

@@ -66,7 +66,7 @@ const renderer = ({
showMore={showMoreConfig !== null}
templateProps={renderState.templateProps}
toggleRefinement={refine}
searchFacetValues={searchForFacetValues && searchForItems}
searchFacetValues={searchForFacetValues ? searchForItems : () => {}}

This comment has been minimized.

@bobylito

bobylito Apr 19, 2017

Contributor

I think the least impactful is not to change the RefinementList, update the propType and use undefined. If only we had option types 🤓

@iam4x iam4x force-pushed the feat/instantsearch.js/unify-connector-api branch from 0a6dbbf to 2e77e1d Apr 24, 2017

@bobylito
Copy link
Contributor

bobylito left a comment

Looks good 👍

@bobylito bobylito merged commit 1c0458f into feat/instantsearch.js/v2 Apr 24, 2017

1 check failed

deploy/netlify Deploy preview failed.
Details

bobylito added a commit that referenced this pull request Apr 24, 2017

@Haroenv Haroenv deleted the feat/instantsearch.js/unify-connector-api branch Jul 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment