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

BC break in 1.8.15 in currentRefinements + toggle #1569

Closed
JanPetr opened this issue Nov 16, 2016 · 2 comments · Fixed by #1574
Closed

BC break in 1.8.15 in currentRefinements + toggle #1569

JanPetr opened this issue Nov 16, 2016 · 2 comments · Fixed by #1574

Comments

@JanPetr
Copy link
Contributor

JanPetr commented Nov 16, 2016

Do you want to request a feature or report a bug?
Bug / BC break

What is the current behavior?
In version 1.8.14 in toggle widget when the value.on is -2 the currentRefinements widget correctly displays "-2".
In version 1.8.15 the currentRefinements widget displays \-2. Might be because of this?

If the current behavior is a bug, please provide all the steps to reproduce and a minimal
JSFiddle example or a repository on GitHub that we can npm install
and npm start.

1.8.14 - https://jsfiddle.net/JanPetr/oynhy43u/2/
1.8.15 - https://jsfiddle.net/JanPetr/oynhy43u/1/
Try refine On Stock products on both versions and see current refinements.

What is the expected behavior?
To be honest I'm not sure which version is correct. I'd expect the one without \

@vvo
Copy link
Contributor

vvo commented Nov 16, 2016

Yes the value is escaped to allow negative numeric in toggles (#1551). Do you think we should consider this as a BUG? In your example the rendering is cryptic, in reality you would put a label in currentRefinedValues instead of displaying the on/off refinements (which most of the time won't be meaningful labels).

But maybe you mean it's a B/C because people could be relying on this value not being escaped before? The question is: is this public API?

@JanPetr
Copy link
Contributor Author

JanPetr commented Nov 17, 2016

Yes, actually putting the label is the issue. Snippet of production code which broke:

search.addWidget(instantsearch.widgets.currentRefinedValues({
    container: '#ais-filter-tags', 
    transformData: {
        item: function (item) {
            if (item.name == "-2") {
                item.name = 'On Stock';
            }
            return item;
        }
    }
}));

To make it properly they had to change the condition to if (item.name == "\\-2").

I'm OK with going with escaped version. More I wanted to point out the BC break.

vvo pushed a commit that referenced this issue Nov 17, 2016
following #1551, we also now need to unescape in currentRefinedValues
widget because the refinement name is public API.

fixes #1569
@vvo vvo closed this as completed in #1574 Nov 21, 2016
vvo added a commit that referenced this issue Nov 21, 2016
#1574)

following #1551, we also now need to unescape in currentRefinedValues
widget because the refinement name is public API.

fixes #1569
vvo pushed a commit that referenced this issue Dec 14, 2016
<a name="1.9.0"></a>
# [1.9.0](v1.8.16...v1.9.0) (2016-12-14)

### Bug Fixes

* **currentRefinedValues:** unescape disjunctive facet refinement names (#1574) ([9ab65c4](9ab65c4)), closes [#1569](#1569)
* **transformData:** default data is an object when not provided (#1570) ([8eeeeba](8eeeeba)), closes [#1538](#1538)

### Features

* **analytics:** new analytics widget to easily plug search to any analytics service ([09d8fda](09d8fda))
* **retry strategy:** new retry strategy ([afdcc3c](afdcc3c))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants