Skip to content

Allow transform to have supply layout defaults handler#1122

Merged
etpinard merged 4 commits into
masterfrom
transform-supply-layout-defaults
Nov 8, 2016
Merged

Allow transform to have supply layout defaults handler#1122
etpinard merged 4 commits into
masterfrom
transform-supply-layout-defaults

Conversation

@etpinard

@etpinard etpinard commented Nov 8, 2016

Copy link
Copy Markdown
Contributor

a requirement for #1099

cc @rreusser

@etpinard etpinard added this to the v1.20.0 milestone Nov 8, 2016
Comment thread src/plots/plots.js
_module = transformModules[i];

if(_module.supplyLayoutDefaults) {
_module.supplyLayoutDefaults(layoutIn, layoutOut, fullData, transitionData);

Copy link
Copy Markdown
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

rreusser commented Nov 8, 2016

Copy link
Copy Markdown
Contributor

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

@etpinard

etpinard commented Nov 8, 2016

Copy link
Copy Markdown
Contributor Author

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

rreusser commented Nov 8, 2016

Copy link
Copy Markdown
Contributor

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

rreusser commented Nov 8, 2016

Copy link
Copy Markdown
Contributor

screen shot 2016-11-08 at 14 47 07

@etpinard

etpinard commented Nov 8, 2016

Copy link
Copy Markdown
Contributor Author

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

rreusser commented Nov 8, 2016

Copy link
Copy Markdown
Contributor

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

rreusser commented Nov 8, 2016

Copy link
Copy Markdown
Contributor

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

2 participants