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

Merged
merged 11 commits into from
Nov 5, 2020
Merged

feat(hitsPerPage): implement getRenderState and getWidgetRenderState #4532

merged 11 commits into from
Nov 5, 2020

Conversation

shortcuts
Copy link
Member

The test:watch yelled at me to add the object at L798 in connectHitsPerPage-test.ts so please if you have any advices or question about the code let me know

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 12, 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 ac85f2f:

Sandbox Source
InstantSearch.js Configuration

@shortcuts shortcuts changed the title Added getRenderState and getWidgetRenderState method for hitsPerPage widget feat(hitsPerPage): implement getRenderState and getWidgetRenderState Oct 12, 2020
@eunjae-lee eunjae-lee requested a review from a team October 12, 2020 15:56
@ghost ghost requested review from Haroenv and yannickcr and removed request for a team October 12, 2020 15:56
Copy link
Contributor

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

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

looks like a good start! I haven't read the tests yet, seems like they don't run for external PRs, so we need to see them manually

src/connectors/hits-per-page/connectHitsPerPage.ts Outdated Show resolved Hide resolved
src/connectors/hits-per-page/connectHitsPerPage.ts Outdated Show resolved Hide resolved
src/connectors/hits-per-page/connectHitsPerPage.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

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

good job so far!

src/connectors/hits-per-page/connectHitsPerPage.ts Outdated Show resolved Hide resolved
src/connectors/hits-per-page/connectHitsPerPage.ts Outdated Show resolved Hide resolved
src/connectors/hits-per-page/connectHitsPerPage.ts Outdated Show resolved Hide resolved
src/connectors/hits-per-page/connectHitsPerPage.ts Outdated Show resolved Hide resolved
Comment on lines 224 to 231
connectorState.createURLFactory = helperState => value => {
return createURL(
helperState.setQueryParameter(
'hitsPerPage',
!value && value !== 0 ? undefined : value
)
);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this function should probably stay in init for consistency with connectorState.setHitsPerPage, but I'm not sure how other connectors dealt with this yet

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. It can stay in init.

Copy link
Member Author

@shortcuts shortcuts Oct 13, 2020

Choose a reason for hiding this comment

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

I wasn't able to keep it in init because I got a createURLFactory is not a function error due to getWidgetRenderState getting called before init, which is weird because there's no such issues with the other widgets (it looks like). Could you check my new update @eunjae-lee ? We checked with @Haroenv to see if there was an alternative but this was the only workaround that worked.

Copy link
Contributor

Choose a reason for hiding this comment

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

basically the problem is that in the test (similar to breadcrumb) it seems to call getWidgetRenderState before init has happened, which means it will throw, since connectorState is still empty object at that point

@eunjae-lee
Copy link
Contributor

@Haroenv @shortcuts
What do you think about this approach? 3cc8e7f

getRenderState is called before init (https://github.com/algolia/instantsearch.js/blob/feat/getWidgetRenderState-hitsPerPage/src/widgets/index/index.ts#L267:L270)
So do we rely on the fact and let getWidgetRenderState to implement connectorState? Or, do we implement connectorState even before init, but instead, let it have factory-type functions to accept things like helper or state later on?

I pushed my commit to show you for discussion, so we can drop it if we decide to go for the previous implementation.

@shortcuts
Copy link
Member Author

@eunjae-lee

As I'm not that familiar with the logic behind the connectorState, your approach seems cleaner if we want to update widgets later on or if we have the same issue with an other widget, and it's not that different to other widgets.

@Haroenv
Copy link
Contributor

Haroenv commented Oct 14, 2020

your approach is the way to go as far as I can see @eunjae-lee, thanks!

@shortcuts shortcuts marked this pull request as ready for review October 16, 2020 08:00
@shortcuts shortcuts merged commit a8ad96d into algolia:feat/render-state Nov 5, 2020
@shortcuts shortcuts deleted the feat/getWidgetRenderState-hitsPerPage branch November 5, 2020 15:59
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.

5 participants