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(panel): render templates on init with render state #4845

Merged
merged 2 commits into from
Aug 24, 2021

Conversation

Haroenv
Copy link
Contributor

@Haroenv Haroenv commented Aug 24, 2021

Summary

Before this PR the initial render happens before widget init. This doesn't have a huge effect, although it got rendered with just an empty object. This makes things needlessly dynamic (more than the types were saying even, because the Template isn't super strict), and would make a template like header({ widgetParams }) { return widgetParams.attribute } throw, even though with this PR it is possible without conditionals or flashing.

Under very strict conditions this could be construed as a breakign change, although it's closer to a fix, therefore I have classified it as a new feature.

Result

panel's templates always get called with render state, but also with init options. Before this PR it got called with either results or an empty object

Before this PR the initial render happens *before* widget init. This doesn't have a huge effect, although it got rendered with just an empty object. This makes things needlessly dynamic (more than the types were saying even, because the Template isn't super strict), and would make a template like `header({ widgetParams }) { return widgetParams.attribute }` throw, even though with this PR it is possible without conditionals or flashing.

Under very strict conditions this could be construed as a breakign change, although it's closer to a fix, therefore I have classified it as a new feature.
@Haroenv Haroenv requested review from a team, tkrugg and francoischalifour and removed request for a team August 24, 2021 09:07
@codesandbox-ci
Copy link

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

Sandbox Source
InstantSearch.js Configuration

Comment on lines +297 to 317
init(...args) {
const [renderOptions] = args;

if (typeof widget.dispose === 'function') {
return widget.dispose.call(this, ...args);
}
const options = {
...(widget.getWidgetRenderState
? widget.getWidgetRenderState(renderOptions)
: {}),
...renderOptions,
};

return undefined;
renderPanel({
options,
hidden: true,
collapsible,
collapsed: false,
});

if (typeof widget.init === 'function') {
widget.init.call(this, ...args);
}
},
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 is the main change, the rest is mostly types and tests

@codesandbox-ci
Copy link

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

Sandbox Source
InstantSearch.js Configuration

Copy link
Member

@francoischalifour francoischalifour left a comment

Choose a reason for hiding this comment

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

I think we initially wanted to display the header and footer before the widget's init method is called, but I don't really see why. Having consistent data passed to the templates is more important imo. Besides, the panel widget is not a side effect anymore.

@Haroenv Haroenv merged commit 0e151a9 into master Aug 24, 2021
@Haroenv Haroenv deleted the feat/panel-templates-init branch August 24, 2021 12:52
@Haroenv
Copy link
Contributor Author

Haroenv commented Aug 24, 2021

I think originally it might have made sense together with the hidden function, as that one only gets called with results, but still it was inconsistent as it either got called with empty object, either with the results, the templates become less conditional with this PR :)

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.

2 participants