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

Refactor out merge and color helpers #5957

Closed
wants to merge 1 commit into from

Conversation

benmccann
Copy link
Contributor

@benmccann benmccann commented Jan 5, 2019

This change better organizes the code and will allow making ./core/core.helpers importable. Making the build entirely importable will allow us to utilize optional dependencies in rollup (example), which will enable users to use moment or luxon, etc.

After this change, the methods in ./core/core.helpers can start to be moved into the helpers directory so that they are included in ./helpers/index. This change is necessary to prevent circular dependencies when making that improvement (see newly added code comment for explanation)

@benmccann benmccann requested a review from simonbrunel January 5, 2019 20:57
@benmccann benmccann changed the title Refactor out color helpers Refactor out merge and color helpers Jan 5, 2019
@kurkle
Copy link
Member

kurkle commented Jan 10, 2019

I tend to agree with @simonbrunel #5959 (comment):

For example configMerge and scaleMerge are not "helpers" but complex logic proper to our main controller and should certainly be moved in core/core.controller.js with a deprecated aliases.

color OTOH could be useful externally, maybe even in its own namespace?

@benmccann
Copy link
Contributor Author

I think @simonbrunel is going to take a stab at implementing this, so I'll close this PR for now. @simonbrunel hopefully this PR helps illuminate the possible circular dependency issue you might run into and one possible solution. Hope it helps

@benmccann benmccann closed this Jan 10, 2019
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