Refactor targets (ready) #78

Merged
merged 5 commits into from Mar 27, 2013

Projects

None yet

2 participants

Contributor
sym3tri commented Mar 26, 2013
  • Added missing features to d3-ext.size(), height(), width()
  • Removed hardcoded target values from components, to remove assumptions and work with other layouts better.
  • Renamed gl-framed to gl-main
  • Instead of putting the container name in the css class attr, moved it to gl-container-name attr.
  • Made default stroke width 1.5, per discussion with UX.
  • Added d3.selection.prototype.selectAttr for convenience.
  • Fixed bug in d3-ext.util.applyTarget()
  • Cleaned up some of the silly layout manager tests.
@amrutac amrutac commented on the diff Mar 27, 2013
examples/layouts.html
@@ -60,37 +60,39 @@
.data(dataConfig)
.render('#container');
- var lineGraph2 = glimpse.graphBuilder.create('line')
+ var lineGraph2 = glimpse.graph()
amrutac
amrutac Mar 27, 2013 Contributor

Is this cause you want just one component per example?

sym3tri
sym3tri Mar 27, 2013 Contributor

It's b/c with these layouts it doesn't make sense to auto-add the avg/min/max like the graphBuilder does.

amrutac
amrutac Mar 27, 2013 Contributor

Oh OK.

@amrutac amrutac commented on the diff Mar 27, 2013
src/d3-ext/select-attr.js
+ * certain value.
+ */
+define([
+ 'd3'
+],
+function(d3) {
+ 'use strict';
+
+ /**
+ * Selects a sub-selection of the current selection which is the first
+ * node that has a matching attr and value.
+ * @param attr The attribute to check.
+ * @param value The value that attr must have set.
+ * @return {d3.selection}
+ */
+ d3.selection.prototype.selectAttr = function(attr, value) {
amrutac
amrutac Mar 27, 2013 Contributor

+1

@amrutac amrutac commented on the diff Mar 27, 2013
src/layout/layoutmanager.js
@@ -193,15 +193,18 @@ function (layouts, array) {
node = getPaddingContainer(node, nodeInfo);
node.attr({
- 'class': nodeInfo['class'],
+ 'gl-container-name': nodeInfo.name,
amrutac
amrutac Mar 27, 2013 Contributor

What's the reason for putting this in the gl-container-name attr instead of class?

sym3tri
sym3tri Mar 27, 2013 Contributor

It never really made sense to have this as a class. It implies that duplicates are ok, meaning that you could render to multiple containers (which we don't support).

I spoke to Johann about this before and he agreed that we should move it to a custom attribute too.

amrutac
amrutac Mar 27, 2013 Contributor

Didn't think about the duplicates issue, good point! 👍

@amrutac amrutac commented on an outdated diff Mar 27, 2013
src/d3-ext/size.js
@@ -5,19 +5,57 @@
define(['d3'], function(d3) {
'use strict';
+ function isGnode(selection) {
amrutac
amrutac Mar 27, 2013 Contributor

Missing docs

@amrutac amrutac commented on an outdated diff Mar 27, 2013
src/d3-ext/size.js
@@ -5,19 +5,57 @@
define(['d3'], function(d3) {
'use strict';
+ function isGnode(selection) {
+ return !selection.empty() && selection.node().tagName === 'g';
+ }
+
+ function lazyAddLayoutRect(selection) {
amrutac
amrutac Mar 27, 2013 Contributor

missing docs

@amrutac amrutac commented on an outdated diff Mar 27, 2013
src/d3-ext/size.js
/**
* d3 selection width
* Returns width attribute of a non-group element.
* If element is a group,
* it returns the 'gl-width' attribute, if it's defined.
* else it returns the bounding box width.
+ * @param {Number} h
amrutac
amrutac Mar 27, 2013 Contributor

@param {Number} w

@amrutac amrutac commented on the diff Mar 27, 2013
src/graphs/graph.js
if (!components_) {
return;
}
components_.forEach(function(component) {
- renderComponent_(component, selection);
+ component.render(root_);
amrutac
amrutac Mar 27, 2013 Contributor

Shouldn't this be component.render(selection.select(component.config('target')) || root_)

sym3tri
sym3tri Mar 27, 2013 Contributor

So I was thinking about this a lot yesterday, and the target property actually only makes sense within the scope of a selection. So in this case our selection is root_ and the target points to another container within that root. If we allow rendering to targets without a root selection then target-based rendering won't work with multiple graphs on the page b/c it loses it's scope. We've only managed to avoid this problem by chance.

So I basically made the selection param passed to components.*.render(selection) required in all cases, and the config.target (aka sub-selector) is now optional. That being the case, I now always pass the root_ to component.render(). The extracted code in d3-ext.util.applyTarget handles narrowing to the sub-selection if a target is specified, otherwise it just renders to the passed selection param.

TLDR
selection and target used to both be optional. Now selection is required when calling render() to avoid scoping issues.

amrutac
amrutac Mar 27, 2013 Contributor

Makes sense.

@amrutac amrutac commented on the diff Mar 27, 2013
src/graphs/graph.js
@@ -132,6 +127,9 @@ function(obj, config, array, assetLoader, format, components, layoutManager,
if (component.yScale) {
component.yScale(config_.yScale);
}
+ if (!component.config('target')) {
+ component.config('target', config_.primaryContainer);
+ }
amrutac
amrutac Mar 27, 2013 Contributor

If the target of the component is not specified, shouldn't it just be rendered in the main svg element?

sym3tri
sym3tri Mar 27, 2013 Contributor

We could do it that way, but my goal here was trying to remove hardcoded coupling between our "Reach" layout and the way the components/graphs work. For example, before all the line components had config.target defaulted (and hardcoded) to .gl-framed, meaning that if we ever changed the layout, everything would break. So I removed it.

Now if we default to the svg element we must specify target in every graph.component() call that we make, which becomes cumbersome, so I added the primaryContainer config to be used as the default. Maybe I should check that the primary container config actually exists before setting it on components, and if not I can leave the component target as null which will force rendering to the root svg. Does that sounds like a good solution to you?

amrutac
amrutac Mar 27, 2013 Contributor

Sounds good!

sym3tri
sym3tri Mar 27, 2013 Contributor

Actually I just realized that if primaryContainer is not set it will just be null, so in that case it the target property will be ignored, and the components will get rendered to the root. I think the code is ok as is.

Contributor
amrutac commented Mar 27, 2013

Nice work with the refactoring! Other than the minor docs related changes LGTM.

@sym3tri sym3tri merged commit 96b16a8 into master Mar 27, 2013
@sym3tri sym3tri deleted the refactor-targets branch Mar 27, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment