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(sortBy): implement getRenderState and getWidgetRenderState #4568

Merged
merged 2 commits into from Nov 4, 2020
Merged

feat(sortBy): implement getRenderState and getWidgetRenderState #4568

merged 2 commits into from Nov 4, 2020

Conversation

shortcuts
Copy link
Member

This implements the getRenderState and getWidgetRenderState widget lifecycle hooks in sortBy.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 3, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit a5b1030:

Sandbox Source
InstantSearch.js Configuration

});

sortBy.init(createInitOptions({ helper }));
sortBy.getWidgetRenderState({ helper }).refine('index_desc');
Copy link
Contributor

Choose a reason for hiding this comment

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

We used to have this pattern in tests:

      const { refine } = renderFn.mock.calls[renderFn.mock.calls.length - 1][0];
      refine('index_desc');

It reads what's rendered from init call and get refine from there.
However, getting refine from getWidgetRenderState() also looks okay to me.

Just leaving this comment for Clément's information, and to see if others think differently.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the heads up!

You're right, I think I saw it in an other connector but now that we have getWidgetRenderState it might be easier to understand (at least for me 🤔 ).

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, I think using getWidgetRenderState is the way to go for new tests I think. It's okay if older tests still use refine from the calls, since the implementation should be the same anyway 👍

Copy link
Member

Choose a reason for hiding this comment

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

Since render state is now public API it makes sense to use it—and it's such a relief!

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, but would like to get @yannickcr's opinion too

@eunjae-lee eunjae-lee merged commit 97b13c2 into algolia:feat/render-state Nov 4, 2020
@shortcuts shortcuts deleted the feat/getWidgetRenderState-sortBy branch November 4, 2020 09:46
@@ -103,7 +103,8 @@ export default function connectSortBy(renderFn, unmountFn = noop) {
return {
$$type: 'ais.sortBy',

init({ helper, instantSearchInstance, parent }) {
init(initOptions) {
const { helper, instantSearchInstance, parent } = initOptions;
const currentIndex = helper.state.index;
Copy link
Member

Choose a reason for hiding this comment

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

We should get the current index from getWidgetRenderState otherwise we can get inconsistencies. We compute it twice at different spots here and we don't prevent the logic from differing.

Haroenv pushed a commit that referenced this pull request Nov 30, 2020
…4568)

* feat(sortBy): implement `getRenderState` and `getWidgetRenderState`

* feat(sortBy): implement `getRenderState` and `getWidgetRenderState`
Haroenv pushed a commit that referenced this pull request Nov 30, 2020
…4568)

* feat(sortBy): implement `getRenderState` and `getWidgetRenderState`

* feat(sortBy): implement `getRenderState` and `getWidgetRenderState`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants