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.

Sign up for GitHub

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

Jump to bottom

feat(clearAll): New widget #427

Merged
merged 1 commit into from
Nov 12, 2015
Merged

feat(clearAll): New widget #427

merged 1 commit into from
Nov 12, 2015

Conversation

Jerska
Copy link
Member

@Jerska Jerska commented Nov 3, 2015

Uses #407 's autoHideContainer instead of hideContainerWhenNoResults. We might want to wait until this is implemented in all widgets before merging.

Closes #405

Jerska
Jerska reviewed Nov 3, 2015
View reviewed changes
widgets/clear-all/clear-all.js
|| !isEmpty(helper.state.disjunctiveFacetsRefinements)
|| !isEmpty(helper.state.numericRefinements)
|| !isEmpty(helper.state.tagRefinements)
|| !isEmpty(helper.state.hierarchicalFacetsRefinements);
Copy link
Member Author

Choose a reason for hiding this comment

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

This is going to be replaced by algolia/algoliasearch-helper-js#195 .

@pixelastic
Copy link
Contributor

This is also missing tests

@Jerska
Copy link
Member Author

Jerska commented Nov 3, 2015

Expected, I'm waiting for the helper update before implementing them (hence the do not merge).

@vvo
Copy link
Contributor

vvo commented Nov 3, 2015

I'm waiting for the helper update before implementing them

Which update? I mean do you just need a release of the helper?

@Jerska
Copy link
Member Author

Jerska commented Nov 3, 2015

No I (or we) need to fulfill algolia/algoliasearch-helper-js#195 before going further. Relying on state.XXXRefinements is highly risky : at the beginning we only had facetsRefinements and disjunctiveFacetsRefinements, now we have 6 of them. Knowing that we need to update one method on the helper when adding a new type of refinements seems way safer than having to remember to update instantsearch.js at the same time on all widgets which require these.

@vvo
Copy link
Contributor

vvo commented Nov 3, 2015

No I (or we) need to fulfill algolia/algoliasearch-helper-js#195 before going further.

If it's right now working we can move on because we are not in the way to add any new type of refinements to the helper in the coming weeks.

Would be good to iterate on this widget and then implement in the helper and then refactor.

@Jerska
Copy link
Member Author

Jerska commented Nov 4, 2015

@vvo , I will actually need to fullfill algolia/algoliasearch-helper-js#195 for currentRefinedValues, so let's not create tests and everything for something that will be replaced just afterwards.

@Jerska
Copy link
Member Author

Jerska commented Nov 4, 2015

Ready for review

If it's right now working we can move on because we are not in the way to add any new type of refinements to the helper in the coming weeks.

@Jerska
Copy link
Member Author

Jerska commented Nov 4, 2015

Actually, I didn't update the documentation, working on this now.

@pixelastic pixelastic mentioned this pull request Nov 5, 2015
Closed
4 tasks
bobylito
bobylito reviewed Nov 6, 2015
View reviewed changes
widgets/clear-all/clear-all.js
let containerNode = getContainerNode(container);
let usage = 'Usage: toggle({container[, cssClasses.{root,header,body,footer,link}, templates.{header,link,footer}, autoHideContainer]})';

let ClearAll = headerFooter(require('../../components/ClearAll/ClearAll.js'));
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 need a header and a footer for a link?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd say yes because it can definetely be included in the side bar with other facets widgets and you might want to style them all the same way.

@Jerska
Copy link
Member Author

Jerska commented Nov 6, 2015

Forgot to clearTags and to add CSS empty classes.

@Jerska
Copy link
Member Author

Jerska commented Nov 12, 2015

Ready for review

@redox
Copy link
Contributor

redox commented Nov 12, 2015

@jerskouille could you maybe squash the commits together to make it cleaner?

redox
redox reviewed Nov 12, 2015
View reviewed changes
lib/main.js
@@ -20,7 +20,8 @@ instantsearch.widgets = {
searchBox: require('../widgets/search-box/search-box'),
rangeSlider: require('../widgets/range-slider/range-slider'),
stats: require('../widgets/stats/stats'),
toggle: require('../widgets/toggle/toggle')
toggle: require('../widgets/toggle/toggle'),
clearAll: require('../widgets/clear-all/clear-all.js')
Copy link
Contributor

Choose a reason for hiding this comment

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

We tried to keep that list ordered; do you mind moving it to the c ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, didn't see that, sure.

@redox
Copy link
Contributor

redox commented Nov 12, 2015

Otherwise, looks 👍 @jerskouille -> once you've done the updates I'm happy to merge it!

@Jerska
Copy link
Member Author

Jerska commented Nov 12, 2015

Done

@redox
Copy link
Contributor

redox commented Nov 12, 2015

Oh btw, do you manage to render it from the Documentation page; because since we don't have any active refinements I assume it auto-hides?

@Jerska
Copy link
Member Author

Jerska commented Nov 12, 2015

What code are we using to instantiate the widget, the one in {% highlight javascript %}... ? If yes, I'll just add autoHideContainer: false.

@Jerska
Copy link
Member Author

Jerska commented Nov 12, 2015

Checking right now

@Jerska
feat(clearAll): New widget
9e61a14 
- ClearAll: Component + tests
- clearAll: Widget + tests
- Documentation
- Default style
@Jerska
Copy link
Member Author

Jerska commented Nov 12, 2015

Done, displaying in the docs.

@redox
Copy link
Contributor

redox commented Nov 12, 2015

👍

redox added a commit that referenced this pull request Nov 12, 2015
@redox
Merge pull request #427 from algolia/feat/clearAll
a9fc622 
feat(clearAll): New widget
@redox redox merged commit a9fc622 into develop Nov 12, 2015
@redox redox deleted the feat/clearAll branch November 12, 2015 15:55
@redox redox removed the in progress label Nov 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Jerska @pixelastic @vvo @redox @bobylito