Skip to content
This repository has been archived by the owner on Feb 19, 2022. It is now read-only.

Export helper methods as named exports #574

Merged
merged 2 commits into from
Feb 21, 2018

Conversation

eysteinn13
Copy link
Contributor

Issue: #650.
See #168 for a very similar PR on the Victory-Pie project.

  • Export all methods individually as named exports, instead of export all helpers as a default export in the form of an object
  • Reorder the method definitions to counter eslint's no-use-before-define errors. This makes the diff look mighty weird, so it might be easier to look at the individual commits' diff

We could not find a clean way to convert classes that were tested using sinon.stub() so we did not refactor them

@eysteinn13 eysteinn13 changed the title Export helper methods as named exports WIP: Export helper methods as named exports Feb 19, 2018
@boygirl
Copy link
Contributor

boygirl commented Feb 19, 2018

@eysteinn13 Thank you for taking on this chore. One change that needs to be made throughout:

things like

static getBaseProps = partialRight(getBaseProps.bind(getBaseProps), fallbackProps);

The bind is unnecessary now as you are no longer relying on context in the helpers (i.e. this.getCalculatedValues())

Another small change I would like to see:

In helpers that export several methods, I would like to see the exports all together at the bottom of the file like:

export default {
  getDomain, 
  getData, 
  getBaseProps
}

You can define them wherever you like in the file, but having the export statement at the bottom like that makes it much clearer what is being exported.

I will merge this work with those changes

@@ -58,7 +58,7 @@ class VictoryArea extends React.Component {
static defaultPolarTransitions = DefaultTransitions.continuousPolarTransitions();
static getDomain = Domain.getDomainWithZero.bind(Domain);
static getData = Data.getData.bind(Data);
static getBaseProps = partialRight(AreaHelpers.getBaseProps.bind(AreaHelpers), fallbackProps);
static getBaseProps = partialRight(getBaseProps.bind(getBaseProps), fallbackProps);
Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary bind

@boygirl
Copy link
Contributor

boygirl commented Feb 19, 2018

Also, a convention for next time: please use a branch with a descriptive name rather than submitting a PR from your master branch

@eysteinn13 eysteinn13 changed the title WIP: Export helper methods as named exports Export helper methods as named exports Feb 20, 2018
@boygirl
Copy link
Contributor

boygirl commented Feb 21, 2018

@eysteinn13 thank you for these changes :)

@boygirl boygirl merged commit 6189b98 into FormidableLabs:master Feb 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants