Refactor domain label (ready) #79

Merged
merged 3 commits into from Mar 29, 2013

Conversation

Projects
None yet
2 participants
@sym3tri
Contributor

sym3tri commented Mar 27, 2013

  • Configure each component's hiddenStates instead of hardcoding when changing graph state.
  • Extracted the graph domain label into its own component.
  • Removed all domain label related hardcoding from graph.js.
  • Add domain label in graphBuilder instead.
  • Created separate formatters in core.format for UTC and local time. Also added one for non-time scale usage.
  • Moved hardcoded domain-label suffix into a config option.
  • Other minor code formatting.
+ return domainLabel();
+ };
+
+});

This comment has been minimized.

Show comment Hide comment
@amrutac

amrutac Mar 28, 2013

Contributor

+1, all that logic in graph.js did not make sense.

@amrutac

amrutac Mar 28, 2013

Contributor

+1, all that logic in graph.js did not make sense.

This comment has been minimized.

Show comment Hide comment
@sym3tri

sym3tri Mar 28, 2013

Contributor

Yeah, I don't know why I put it there in the 1st place.

@sym3tri

sym3tri Mar 28, 2013

Contributor

Yeah, I don't know why I put it there in the 1st place.

@@ -30,6 +31,8 @@ function(graphBuilder, graph) {
beforeEach(function() {
testGraph = graphBuilder.create('line');
testGraph.config('xScale', d3.scale.linear());
+ testGraph.component('gl-domain-label')
+ .config('formatter', format.standardDomain);
testGraph.data().add(testData);
});

This comment has been minimized.

Show comment Hide comment
@amrutac

amrutac Mar 28, 2013

Contributor

Maybe add a test here to check if the domain label is getting added?

@amrutac

amrutac Mar 28, 2013

Contributor

Maybe add a test here to check if the domain label is getting added?

This comment has been minimized.

Show comment Hide comment
@sym3tri

sym3tri Mar 29, 2013

Contributor
}
+
};
});

This comment has been minimized.

Show comment Hide comment
@amrutac

amrutac Mar 28, 2013

Contributor

Looks like we dont have any tests for this

@amrutac

amrutac Mar 28, 2013

Contributor

Looks like we dont have any tests for this

This comment has been minimized.

Show comment Hide comment
@sym3tri

sym3tri Mar 29, 2013

Contributor
@amrutac

This comment has been minimized.

Show comment Hide comment
@amrutac

amrutac Mar 29, 2013

Contributor

LGTM

Contributor

amrutac commented Mar 29, 2013

LGTM

Ed Rooth
refactor(*) Pulled domain label out of graph and into its own component.
- Removed all related hardcoded things from graph.js.
- Added missing tests

sym3tri pushed a commit that referenced this pull request Mar 29, 2013

@sym3tri sym3tri merged commit 11e59e2 into master Mar 29, 2013

@sym3tri sym3tri deleted the refactor-domain-label branch Mar 29, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment