Skip to content
This repository has been archived by the owner on Dec 30, 2022. It is now read-only.

feat(index): add multi-index widget #745

Merged
merged 19 commits into from
Jan 10, 2020
Merged

feat(index): add multi-index widget #745

merged 19 commits into from
Jan 10, 2020

Conversation

Haroenv
Copy link
Contributor

@Haroenv Haroenv commented Nov 7, 2019 •
edited
Loading

This new widget <ais-index> allows you to search through multiple indices at once, without manually needing to keep parameters in sync.

BREAKING CHANGE: this introduces the dependency on InstantSearch v4, which has its own breaking changes, you might see if you're using methods on helper. See more here: https://www.algolia.com/doc/guides/building-search-ui/upgrade-guides/js/#upgrade-from-v3-to-v4

Haroenv
Haroenv commented Nov 7, 2019
View reviewed changes
src/components/Index.js Show resolved Hide resolved
src/mixins/widget.js Outdated Show resolved Hide resolved
src/mixins/widget.js Outdated Show resolved Hide resolved
stories/Autocomplete.stories.js Show resolved Hide resolved
@Haroenv Haroenv force-pushed the feat/index-widget branch 4 times, most recently from 1eefc8a to cf5dbf5 Compare November 20, 2019 11:35
@Haroenv
chore: update to InstantSearch.js v4
cf370c1 
BREAKING CHANGE: see migration guide, changes are mostly related to helper usage

chore(bundlesize): update for ISv4
@Haroenv
feat(autocomplete): switch index prop for ais-index
c34b673 
BREAKING CHANGE: replace the index prop by additional `ais-index` widgets as sibling
@Haroenv
feat(widgetMixin): allow injected index widget
56a5637 
test(widgetMixin): allow for multi-index
@Haroenv
feat(ais-index): add widget
c7cc494 
test(index): add basic tests

fix: move to $ais_ for provides

docs: add index widget stories

chore: correct previous commit
@Haroenv
test: provide/inject
780a322
@Haroenv Haroenv requested a review from a team November 20, 2019 14:26
@ghost ghost requested review from tkrugg and yannickcr and removed request for a team November 20, 2019 14:26
francoischalifour
francoischalifour reviewed Nov 20, 2019
View reviewed changes
package.json Outdated Show resolved Hide resolved
src/components/Index.js Show resolved Hide resolved
src/components/Index.js Outdated Show resolved Hide resolved
src/components/Index.js Outdated Show resolved Hide resolved
@eunjae-lee eunjae-lee self-requested a review November 29, 2019 09:59
@Haroenv Haroenv removed the request for review from tkrugg November 29, 2019 13:42
@Haroenv
Copy link
Contributor Author

Haroenv commented Nov 29, 2019

I don't understand why the e2e tests fail still :( @yannickcr

@yannickcr
Copy link
Contributor

@Haroenv Seems that there is an issue with the router.
In https://app.saucelabs.com/tests/79198096cecc4c0087e8cc09d942705f the parameters from the URLs are not applied to the widgets.
And in https://app.saucelabs.com/tests/7ba577349b0d462eb1366cd5bf11ace9 the URL is not updated when navigating in categories/pages.

@Haroenv
Copy link
Contributor Author

Haroenv commented Nov 29, 2019

hmm, that's weird since Vue InstantSearch isn't doing anything special with the routing there that InstantSearch wouldn't also have

francoischalifour
francoischalifour reviewed Dec 2, 2019
View reviewed changes
Copy link
Member

@francoischalifour francoischalifour left a comment

Choose a reason for hiding this comment

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

I feel like we'd really benefit from changing our test stack. The tests are really hard to understand, and we never use the components like we do when using the library.

src/mixins/widget.js Outdated Show resolved Hide resolved
@eunjae-lee
Copy link
Contributor

This seems good to me. Is there any part of the code you want us to look deeply? Or anything to test out?

yannickcr
yannickcr reviewed Dec 11, 2019
View reviewed changes
Copy link
Contributor

@yannickcr yannickcr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

A small question but beside that LGTM.

But I find the tests really unreadable (too many mocks and checks on internals IMHO). Like @francoischalifour I think we should change the way we tests our components.

PS: I see that the e2e tests are still failing, did you find the issue about it?

src/mixins/widget.js Outdated Show resolved Hide resolved
stories/Autocomplete.stories.js Show resolved Hide resolved
francoischalifour
francoischalifour approved these changes Dec 13, 2019
View reviewed changes
Copy link
Member

@francoischalifour francoischalifour left a comment

Choose a reason for hiding this comment

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

Did you try running the e-commerce demo with this version?

We should probably change the base branch to a next branch.

src/components/Index.js Outdated Show resolved Hide resolved
src/components/__tests__/Index-integration.js Outdated Show resolved Hide resolved
src/components/__tests__/Index-integration.js Outdated Show resolved Hide resolved
src/components/__tests__/InstantSearch-integration.js Outdated Show resolved Hide resolved
@Haroenv Haroenv mentioned this pull request Dec 20, 2019
Merged
@kidonng kidonng mentioned this pull request Dec 31, 2019
Closed
@Haroenv Haroenv changed the base branch from master to next January 9, 2020 13:50
eunjae-lee
eunjae-lee approved these changes Jan 10, 2020
View reviewed changes
Copy link
Contributor

@eunjae-lee eunjae-lee left a comment

Choose a reason for hiding this comment

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

looks good to me

francoischalifour
francoischalifour approved these changes Jan 10, 2020
View reviewed changes
src/components/__tests__/Index-integration.js Outdated Show resolved Hide resolved
stories/Index.stories.js Outdated Show resolved Hide resolved
@Haroenv @francoischalifour
chore: wording in tests & stories
6234bcf 
Co-Authored-By: François Chalifour <francoischalifour@users.noreply.github.com>
@Haroenv Haroenv merged commit 3c32cfe into next Jan 10, 2020
@Haroenv Haroenv deleted the feat/index-widget branch March 5, 2020 08:19
@Haroenv Haroenv mentioned this pull request Mar 5, 2020
Closed
Haroenv added a commit to algolia/instantsearch that referenced this pull request Dec 28, 2022
@Haroenv
feat(index): add multi-index widget (algolia/vue-instantsearch#745)
9970a91 
This new widget <ais-index> allows you to search through multiple indices at once, without manually needing to keep parameters in sync.

BREAKING CHANGE: this introduces the dependency on InstantSearch v4, which has its own breaking changes, you might see if you're using methods on helper. See more here: https://www.algolia.com/doc/guides/building-search-ui/upgrade-guides/js/#upgrade-from-v3-to-v4
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@posva posva posva left review comments

@francoischalifour francoischalifour francoischalifour approved these changes

@eunjae-lee eunjae-lee eunjae-lee approved these changes

@yannickcr yannickcr yannickcr approved these changes

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
@Haroenv @yannickcr @eunjae-lee @posva @francoischalifour