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

Allow transform to have supply layout defaults handler #1122

Merged
merged 4 commits into from
Nov 8, 2016

Conversation

etpinard
Copy link
Contributor

@etpinard etpinard commented Nov 8, 2016

a requirement for #1099

cc @rreusser

@etpinard etpinard added this to the v1.20.0 milestone Nov 8, 2016
_module = transformModules[i];

if(_module.supplyLayoutDefaults) {
_module.supplyLayoutDefaults(layoutIn, layoutOut, fullData, transitionData);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rreusser I chose to only pass transitionData to transform module supplyLayoutDefaults, because that's the only place where we need them at the moment.

@rreusser
Copy link
Contributor

rreusser commented Nov 8, 2016

Looks good to me. Only exception is in plot_api/register.js. Can we add hasSupplyLayoutDefaults to the check?

@etpinard
Copy link
Contributor Author

etpinard commented Nov 8, 2016

Can we add hasSupplyLayoutDefaults to the check?

By check, you mean something like:

if(typeof newModule.supplyLayoutDefaults !== 'function') {
  throw new Error('supplyLayoutDefaults method is not a function');
}

?

Otherwise, supplyLayoutDefaults method should be very much optional.

@rreusser
Copy link
Contributor

rreusser commented Nov 8, 2016

I was referring to the fact that a transform that only has supplyLayoutDefaults (as this one does) fails unless it has a dummy no-op for either calcTransform or transform. That's easy enough at least. I have no strong feelings here.

@rreusser
Copy link
Contributor

rreusser commented Nov 8, 2016

screen shot 2016-11-08 at 14 47 07

@etpinard
Copy link
Contributor Author

etpinard commented Nov 8, 2016

unless it has a dummy no-op for either calcTransform or transform.

I would prefer keeping this as such. Transform modules should transform the data using .transform or .calcTransform.

@rreusser
Copy link
Contributor

rreusser commented Nov 8, 2016

Agreed, though the current use-case is an exception to that rule. I give it the 💃 though.

@etpinard etpinard merged commit d3c59da into master Nov 8, 2016
@etpinard etpinard deleted the transform-supply-layout-defaults branch November 8, 2016 21:09
@rreusser
Copy link
Contributor

rreusser commented Nov 8, 2016

🎉 thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants