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

feat: Implement transformItems API #3042

Merged
merged 26 commits into from
Aug 7, 2018
Merged

feat: Implement transformItems API #3042

merged 26 commits into from
Aug 7, 2018

Conversation

francoischalifour
Copy link
Member

@francoischalifour francoischalifour commented Jul 23, 2018

This adds support for the transformItems option on widgets.

How

The implementation was made at the connector level for the following reasons:

  • The transformation takes place before the rendering
  • Leverage the transformItems API in all flavors (since the connectors are/will be shared across InstantSearch libraries)

How to review

Each commit is a widget implementation. Click on the commit and add your review there.

See commits →

Supported widgets

These widgets have been selected based on whether they contain items and based on the current React InstantSearch API. Let me know if this list should be updated.

See all InstantSearch.js widgets →

React InstantSearch widgets implementing transformItems
  • Breadcrumb
  • ClearRefinements
  • CurrentRefinements
  • HierarchicalMenu
  • HitsPerPage
  • Menu
  • MenuSelect
  • NumericMenu
  • RefinementList
  • SortBy

Closes #2949

@algobot
Copy link
Contributor

algobot commented Jul 23, 2018

Deploy preview for algolia-instantsearch ready!

Built with commit 7c10a17

https://deploy-preview-3042--algolia-instantsearch.netlify.com

@francoischalifour francoischalifour force-pushed the feat/transform-items branch 3 times, most recently from c49dfb2 to cfcc2c0 Compare July 25, 2018 14:30
@francoischalifour francoischalifour requested a review from a team July 25, 2018 14:33
@francoischalifour
Copy link
Member Author

Ready for review @algolia/instantsearch-for-websites.

I can cherry pick my commits to create several PRs if it makes it easier.

@francoischalifour francoischalifour changed the base branch from feat/3.0 to develop July 25, 2018 14:38
@francoischalifour francoischalifour force-pushed the feat/transform-items branch 2 times, most recently from 16ec292 to c2bddd2 Compare July 25, 2018 14:44
Copy link
Contributor

@bobylito bobylito left a comment

Choose a reason for hiding this comment

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

Look goods. I have some questions on the hits/infiniteHits implementations. Also could you remove sinon from the test files you've modified? That'd be awesome :)

@@ -192,7 +194,7 @@ The first one will be picked, you should probably set only one default value`
);
},

_transformItems({ hitsPerPage }) {
_normalizeItems({ hitsPerPage }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes good move 👍

if (widgetParams.escapeHits && results.hits && results.hits.length > 0) {
results.hits = escapeHits(results.hits);
if (results.hits && results.hits.length > 0) {
if (typeof widgetParams.transformItems === 'function') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not provide a default implementation (identity) like in other widget and remove this if statement? Also I'd suggest to test the type sooner at the initialization.

Copy link
Member Author

Choose a reason for hiding this comment

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

Glad you caught that. I wasn't sure if I should have updated the code flow of the connector or should have adapted my code on the existing flow.

) {
results.hits = escapeHits(results.hits);
if (results.hits && results.hits.length > 0) {
if (typeof widgetParams.transformItems === 'function') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same remarks than in the hits connector.

});

const helper = jsHelper({}, '', widget.getConfiguration({}));
helper.search = sinon.stub();
Copy link
Contributor

Choose a reason for hiding this comment

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

When we touch a test file that is still using sinon, we try to take the opportunity to replace it with Jest mocks. Can you do that? 🙏

Copy link
Member Author

Choose a reason for hiding this comment

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

For sure! I initially didn't want to edit too much code as it was intended to be merged on the yet-to-be consequent feat/3.0 branch.

@francoischalifour
Copy link
Member Author

francoischalifour commented Jul 30, 2018

Tests have been migrated to Jest but rely on #3045.

@bobylito
Copy link
Contributor

Tests have been migrated to Jest but rely on #3045.

merged :)

Copy link
Contributor

@bobylito bobylito left a comment

Choose a reason for hiding this comment

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

Good work! Thanks for taking the time to remove sinon. I think we could do better in the usage of jest but that's fine for now.

@bobylito bobylito merged commit 1510a94 into develop Aug 7, 2018
@bobylito bobylito deleted the feat/transform-items branch August 7, 2018 09:03
bobylito pushed a commit that referenced this pull request Aug 7, 2018
<a name=2.10.0></a>
# [2.10.0](v2.9.0...v2.10.0) (2018-08-07)

### Features

* Implement  API ([#3042](#3042)) ([1510a94](1510a94))
bobylito pushed a commit that referenced this pull request Aug 8, 2018
<a name=2.10.0></a>
# [2.10.0](v2.9.0...v2.10.0) (2018-08-08)

### Bug Fixes

* **release:** provide interactive TTY for npm publish ([#3053](#3053)) ([ede9460](ede9460))

### Features

* Implement  API ([#3042](#3042)) ([1510a94](1510a94))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants