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(ts): convert menu #4652

Merged
merged 7 commits into from
Feb 18, 2021
Merged

feat(ts): convert menu #4652

merged 7 commits into from
Feb 18, 2021

Conversation

Haroenv
Copy link
Contributor

@Haroenv Haroenv commented Feb 12, 2021

Summary

converts the menu-related files to typescript

Result

converted:

  • connectMenu
  • menu
  • menuSelect

fixed a bug in connectMenu#getRenderState (forgotten to nest by attribute)

- connectMenu
- menu
- menuSelect
@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 12, 2021

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 9d9657c:

Sandbox Source
InstantSearch.js Configuration

Copy link
Contributor Author

@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.

let toggleShowMore = () => {};
function createToggleShowMore(
renderOptions: RenderOptions,
widget: Widget
Copy link
Contributor Author

Choose a reason for hiding this comment

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

passing the "this" to this function is the most natural method I found for show more. refinementList and hierarchicalMenu aren't yet converted, so this is the first widget with this pattern

menu: this.getWidgetRenderState(renderOptions),
menu: {
...renderState.menu,
[attribute]: this.getWidgetRenderState(renderOptions),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note that this fixes a bug!

Comment on lines +19 to +20
// can not use = {} here, since the template could have different constraints
defaultTemplates?: TTemplates,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not super happy with this, but refinementList for example doesn't pass defaultTemplates at all

Comment on lines +224 to +229
menu: {
[attribute: string]: WidgetRenderState<
MenuRendererOptions,
MenuConnectorParams
>;
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same bugfix, as well as noting its omission in the type

Comment on lines +23 to 24
// @ts-ignore
menuSelect({ container: undefined });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe we should bump the ts version so we can use ts-expect-error?

src/widgets/menu/__tests__/__snapshots__/menu-test.ts.snap Outdated Show resolved Hide resolved
Comment on lines 546 to 547
const renderState1 = menu.getWidgetRenderState(
{},
createInitOptions({ helper })
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this test was wrong :o

Copy link
Contributor

Choose a reason for hiding this comment

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

good job by TS finding it out :)

});
}

if (!this._createURL) {
this._createURL = facetValue =>
createURL(helper.state.toggleRefinement(attribute, facetValue));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this function is deprecated, toggleFacetRefinement its exact replacement

@Haroenv Haroenv marked this pull request as ready for review February 16, 2021 09:47
@Haroenv Haroenv requested review from a team, yannickcr and eunjae-lee and removed request for a team February 16, 2021 09:47
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.

It looks good to me. Congrats on one less widget in JS :)

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.

3 participants