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

Move bar transform functions to bar helpers #4391

Closed
wants to merge 1 commit into from

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Nov 29, 2019

Before addressing #4247
this PR remove bar transform functions namely toMoveInsideBar & toMoveOutsideBar from bar/plot to bar/helpers as those function are required by traces e.g. treemap.
This can also help make bar plot code (which used by histogram, funnel and waterfall) more compact.

@plotly/plotly_js

@etpinard
Copy link
Contributor

I'm not sure I approve. helpers.js files are usually kept from utilities that are used across different methods in a given trace module.

For example, castOption for pies

exports.castOption = function castOption(item, indices) {
if(Array.isArray(item)) return exports.getFirstFilled(item, indices);
else if(item) return item;
};

is called in plot.js and style_one.js

var pull = +helpers.castOption(trace.pull, pt.pts) || 0;

var lineColor = castOption(line.color, pt.pts) || Color.defaultLine;
var lineWidth = castOption(line.width, pt.pts) || 0;


So, I don't see anything wrong with keeping all bar "plotting" subroutines in bar/plot.js. I don't see anything wrong with having treemap require bar/plot.js either. Moving stuff around simply to make things more compact (which is subjective, by the way) is usually a waste of time.

@archmoj
Copy link
Contributor Author

archmoj commented Nov 29, 2019

I see the point that helpers is not the best place for putting these functions.
So maybe we should put them in a separate file (or leave them where they are).
By the way here is a comment from treemap PR which is quite related:
#4185 (comment)

@etpinard
Copy link
Contributor

So maybe we should put them in a separate file (or leave them where they are).

I vote for leaving them in bar/plot.js.

@archmoj archmoj closed this Dec 3, 2019
@archmoj archmoj deleted the mv-bar-transform-functions branch December 3, 2019 18:26
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