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

Merged
merged 2 commits into from
Oct 30, 2020
Merged

feat(menu): implement getRenderState and getWidgetRenderState #4540

merged 2 commits into from
Oct 30, 2020

Conversation

shortcuts
Copy link
Member

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

I'm not too sure about the tests but as usual, any help is welcome!

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 20, 2020 •
edited
Loading

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 dab5b55:

Sandbox Source
InstantSearch.js Configuration

eunjae-lee
eunjae-lee reviewed Oct 20, 2020
View reviewed changes
src/connectors/menu/connectMenu.js
results,
instantSearchInstance,
});
this.toggleShowMore = this.createToggleShowMore(renderOptions);
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, it's better to explicitly pass results and instantSearchInstance.

Copy link
Member Author

@shortcuts shortcuts Oct 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@eunjae-lee I did it this way for readability. The other widgets (refinementList and hierarchicalMenu) are doing it this way too but as they need the whole renderOptions and not just these two variables, I can revert it back if you think it's better.

Haroenv
Haroenv approved these changes Oct 20, 2020
View reviewed changes
src/connectors/menu/connectMenu.js Outdated Show resolved Hide resolved
src/connectors/menu/connectMenu.js
Comment on lines +143 to +144
this._createURL = facetValue =>
createURL(helper.state.toggleRefinement(attribute, facetValue));
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 it's all right to leave this as either part of init or "if it doesn't yet exist" in getWidgetRenderState, not sure what we settled on in the other PR

Copy link
Contributor

Choose a reason for hiding this comment

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

I also kept it in the init in my own PRs.

@shortcuts
Removed duplicated declaration of jsHelper, moved `cachedToggleShow…
dab5b55 
…More` binding to `init` to avoid it happens more than once
@shortcuts shortcuts marked this pull request as ready for review October 22, 2020 12:40
@shortcuts shortcuts merged commit a4fee7f into algolia:feat/render-state Oct 30, 2020
@shortcuts shortcuts deleted the feat/getWidgetRenderState-menu branch October 30, 2020 10:31
Haroenv pushed a commit that referenced this pull request Nov 30, 2020
@shortcuts @Haroenv
feat(menu): implement getRenderState and getWidgetRenderState (#4540
8135cc6 
)

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

* Removed duplicated declaration of `jsHelper`, moved `cachedToggleShowMore` binding to `init` to avoid it happens more than once
Haroenv pushed a commit that referenced this pull request Nov 30, 2020
@shortcuts @Haroenv
feat(menu): implement getRenderState and getWidgetRenderState (#4540
239906c 
)

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

* Removed duplicated declaration of `jsHelper`, moved `cachedToggleShowMore` binding to `init` to avoid it happens more than once
Haroenv added a commit that referenced this pull request Dec 9, 2020
@Haroenv @francoischalifour
@shortcuts @yannickcr
Poc/telemetry3 (#4601)
70ea711 
* feat(core): introduce `getWidgetUiState` lifecycle hook (1/n) (#4454)

This deprecates `getWidgetState` to `getWidgetUiState` to make the upcoming `getWidgetRenderState` less confusing.

* feat(core): introduce `getWidgetRenderState` (2/n) (#4457)

This introduces the widget lifecycle hook `getWidgetRenderState` and implements it for `connectSearchBox`.

* feat(autocomplete): implement `getWidgetRenderState` (#4466)

* feat(breadcrumb): implement `getWidgetRenderState` (#4467)

* feat(clearRefinements): implement `getWidgetRenderState` (#4468)

* feat(configure): implement `getWidgetRenderState` (#4469)

* feat(currentRefinements): implement `getWidgetRenderState` (#4470)

* fix(breadcrumb): add attribute to render state (#4472)

* feat(hierarchicalMenu): implement `getWidgetRenderState` (#4471)

* fix: provide both `getWidgetRenderState` and `getRenderState` in connectors (#4518)

* WIP

* remove temporary implementation

* rename getWidgetRenderState to getRenderState

* fix wrong parameter name

* update panel

* update comment

* chore: remove panel-related code

* fixing types WIP

* fix type

* do not cast the return of getWidgetRenderState

* Revert "do not cast the return of getWidgetRenderState"

This reverts commit 614bc53.

* Revert "fix type"

This reverts commit 54e31fc.

* add TWidgetRenderState to connector

* add generics to Widget for getWidgetRenderState

* fix to allow nullish getWidgetRenderState when unknown

* remove exclamation marks

* remove the type for widgetParams (was experimental)

* make getRenderState optional for widgets with default generics

* update other connectors to follow new connector type

* update types in tests

* add comment

* do not declare individual widget render state types

* feat(hits): implement `getWidgetRenderState` (#4525)

* feat(range): implement `getRenderState` and `getWidgetRenderState` (#4536)

* fix(types): fix type errors (#4537)

* fix type errors

* fix type errors

* simplify test util

* update type of widgetParams

* add lifecycle methods

* fixing breadcrumb widget

* revert the change (let's do this in later iteration)

* add lifecycle methods for places widget

* replace WidgetFactory with Factory

* remove unnecessary part

* rename options to params

* Revert "remove unnecessary part"

This reverts commit 5b2ed14.

* Revert "replace WidgetFactory with Factory"

This reverts commit ee125bd.

* fix getRenderState of places and analytics

* feat(poweredBy): getWidgetRenderState (#4551)

* feat(poweredBy): getWidgetRenderState

DX-206

* fix jsdoc

* feat(menu): implement `getRenderState` and `getWidgetRenderState` (#4540)

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

* Removed duplicated declaration of `jsHelper`, moved `cachedToggleShowMore` binding to `init` to avoid it happens more than once

* feat(renderState): add connectNumericMenu (#4550)

* feat(renderState): add connectNumericMenu

DX-204

* Apply suggestions from code review

Co-authored-by: Yannick Croissant <[email protected]>

* Apply suggestions from code review

Co-authored-by: Yannick Croissant <[email protected]>

* POC: Add telemetry

* Update src/widgets/configure-related-items/configure-related-items.ts

Co-authored-by: Haroen Viaene <[email protected]>

* Use full Schema

* POC: Add telemetry 2

* Use middleware

* move code a little

* chore: fix type of middleware

TODO: make sure this stays on rebase

Co-authored-by: François Chalifour <[email protected]>
Co-authored-by: Eunjae Lee <[email protected]>
Co-authored-by: Yannick Croissant <[email protected]>
Co-authored-by: Clément Vannicatte <[email protected]>
Co-authored-by: Yannick Croissant <[email protected]>
Haroenv added a commit that referenced this pull request Dec 18, 2020
@Haroenv @francoischalifour
@shortcuts @yannickcr
Poc/telemetry3 (#4601)
3c06678 
* feat(core): introduce `getWidgetUiState` lifecycle hook (1/n) (#4454)

This deprecates `getWidgetState` to `getWidgetUiState` to make the upcoming `getWidgetRenderState` less confusing.

* feat(core): introduce `getWidgetRenderState` (2/n) (#4457)

This introduces the widget lifecycle hook `getWidgetRenderState` and implements it for `connectSearchBox`.

* feat(autocomplete): implement `getWidgetRenderState` (#4466)

* feat(breadcrumb): implement `getWidgetRenderState` (#4467)

* feat(clearRefinements): implement `getWidgetRenderState` (#4468)

* feat(configure): implement `getWidgetRenderState` (#4469)

* feat(currentRefinements): implement `getWidgetRenderState` (#4470)

* fix(breadcrumb): add attribute to render state (#4472)

* feat(hierarchicalMenu): implement `getWidgetRenderState` (#4471)

* fix: provide both `getWidgetRenderState` and `getRenderState` in connectors (#4518)

* WIP

* remove temporary implementation

* rename getWidgetRenderState to getRenderState

* fix wrong parameter name

* update panel

* update comment

* chore: remove panel-related code

* fixing types WIP

* fix type

* do not cast the return of getWidgetRenderState

* Revert "do not cast the return of getWidgetRenderState"

This reverts commit 614bc53.

* Revert "fix type"

This reverts commit 54e31fc.

* add TWidgetRenderState to connector

* add generics to Widget for getWidgetRenderState

* fix to allow nullish getWidgetRenderState when unknown

* remove exclamation marks

* remove the type for widgetParams (was experimental)

* make getRenderState optional for widgets with default generics

* update other connectors to follow new connector type

* update types in tests

* add comment

* do not declare individual widget render state types

* feat(hits): implement `getWidgetRenderState` (#4525)

* feat(range): implement `getRenderState` and `getWidgetRenderState` (#4536)

* fix(types): fix type errors (#4537)

* fix type errors

* fix type errors

* simplify test util

* update type of widgetParams

* add lifecycle methods

* fixing breadcrumb widget

* revert the change (let's do this in later iteration)

* add lifecycle methods for places widget

* replace WidgetFactory with Factory

* remove unnecessary part

* rename options to params

* Revert "remove unnecessary part"

This reverts commit 5b2ed14.

* Revert "replace WidgetFactory with Factory"

This reverts commit ee125bd.

* fix getRenderState of places and analytics

* feat(poweredBy): getWidgetRenderState (#4551)

* feat(poweredBy): getWidgetRenderState

DX-206

* fix jsdoc

* feat(menu): implement `getRenderState` and `getWidgetRenderState` (#4540)

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

* Removed duplicated declaration of `jsHelper`, moved `cachedToggleShowMore` binding to `init` to avoid it happens more than once

* feat(renderState): add connectNumericMenu (#4550)

* feat(renderState): add connectNumericMenu

DX-204

* Apply suggestions from code review

Co-authored-by: Yannick Croissant <[email protected]>

* Apply suggestions from code review

Co-authored-by: Yannick Croissant <[email protected]>

* POC: Add telemetry

* Update src/widgets/configure-related-items/configure-related-items.ts

Co-authored-by: Haroen Viaene <[email protected]>

* Use full Schema

* POC: Add telemetry 2

* Use middleware

* move code a little

* chore: fix type of middleware

TODO: make sure this stays on rebase

Co-authored-by: François Chalifour <[email protected]>
Co-authored-by: Eunjae Lee <[email protected]>
Co-authored-by: Yannick Croissant <[email protected]>
Co-authored-by: Clément Vannicatte <[email protected]>
Co-authored-by: Yannick Croissant <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eunjae-lee eunjae-lee eunjae-lee left review comments

@Haroenv Haroenv Haroenv 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.
4 participants
@shortcuts @yannickcr @eunjae-lee @Haroenv