feat(api): add data to CurrentRefinements connector #1550
Conversation
|
By analyzing the blame information on this pull request, we identified @Morhaus, @vvo and @Kikobeats to be potential reviewers |
| return filters.reduce((res, filter) => filter.clear(res), state); | ||
| refine(props, state, items) { | ||
| // `value` corresponds to our internal clear function computed in each connector metadata. | ||
| const clears = items instanceof Array ? items.map(item => item.value) : [items]; |
There was a problem hiding this comment.
I'm very not satisfied of this naming. If someone has a better idea, i'll take it!
The tricky part is that either the user pass us an array of items (to remove all of them), or directly a clear function..
There was a problem hiding this comment.
I do not understand your comment, can you detail a bit more?
There was a problem hiding this comment.
refine here has one purpose: calling the clear function to remove refinements.
We hide this to the end user by passing a clear function as value, then he need to use it like this: props.refine(item.value).
However we have two use cases: either we directly have the clear function if it's a single clear, or we get an array of items, and then the idea is to call each clear function to remove all filters at the same time (clearAll behavior).
I'm just unhappy with the naming I found to express this in the code.
There was a problem hiding this comment.
There's still the naming issue I pointed. It might not be so easy to understand the code. But currently I don't have a better idea for it.
I do not see any BIG issue with it right now, is your naming issue only about "const clears"? Then we can call it "const refinementsToClear"?
|
I voluntarly didn't spent much time to improve our CurrentRefinements widget. We first need to determine what would be a kick-ass CurrentRefinements widget. |
mthuret
force-pushed
the
docs/current-filters-api-change
branch
from
ee3d199 to
ca1ef10
Compare
| @@ -80,7 +80,7 @@ function transformValue(value, limit, props, state) { | |||
| * @propType {string} [separator='>'] - Specifies the level separator used in the data. | |||
| * @propType {string[]} [rootPath=null] - The already selected and hidden path. | |||
| * @propType {boolean} [showParentLevel=true] - Flag to set if the parent level should be displayed. | |||
| * @providedPropType {function} refine - a function to remove a single filter | |||
| * @providedPropType {function} refine - a function to select a refinement | |||
There was a problem hiding this comment.
Does it toggle the value or just select it?
There was a problem hiding this comment.
toggle is more accurate
mthuret
force-pushed
the
docs/current-filters-api-change
branch
from
ca1ef10 to
3462cf0
Compare
|
Are we good here? |
|
There's still the naming issue I pointed. It might not be so easy to understand the code. But currently I don't have a better idea for it. |
BREAKING CHANGE: some data were added to CurrentRefinements connector if our computed label isn't the one wanted. See the docs for more information about the new structure.
mthuret
force-pushed
the
docs/current-filters-api-change
branch
from
3462cf0 to
b810bab
Compare
final items structure: