-
Notifications
You must be signed in to change notification settings - Fork 85
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
Version 0.2 Roadmap #62
Conversation
I haven't had as much time to play with d3.chart lately, but these changes look promising and seem to address needs that arose from real-world use. I particularly like the abstracted data attributes, and I think I'll need to get my hands dirty with the other features to really understand them. Great job |
Hey @jugglinmike this looks really good. One question I have is if the concept of a |
@tbranyen I think that should be minor enough to add in. |
Alright, this issue is now a pull request containing a work-in-progress for version 0.2 . Please note that this branch is intended to facilitate discussion--not for general development. Consider it unstable as we will likely rebase and force-push before finally merging. |
"src/chart-extensions.js" | ||
], | ||
libraries: [ | ||
"node_modules/datamap/src/datamap.js" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we implicitly deciding to manage external packages with npm here? I'm ok with that, but wanted to check that this makes sense going forward too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels kind of gross to me, too, but it seems like the simplest solution to the problem. We already rely on NPM during development--using another package manager would be introducing new development dependencies. I'm open to other approaches, but since this is completely an internal concern (it does not effect consumers of the library in any way), we can change it whenever we want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't have a more elegant solution. I'm ok with this as well tbh, especially since it gets built in. If that weren't the case, maybe we'd have to think about it harder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's important that we don't think too hard about anything in this project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you guys investigate browserify? I know you don't want to lock the project into a module and/or package system, but going the npm/browserify way simplifies things a lot -- and d3.js supports it out-of-the-box. As far as I know this wouldn't lock the users as browserify can generate UMD files (--standalone
). The article of Kyle R. is relevant as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @jeanlauliac! It seems like browserify has been gaining a lot of traction lately. I've used it, but only for a tiny library. It might be just the thing that we need.
True to my word, I've rebased this branch. Here are the latest additions:
That migration guide includes an abbreviated example of the changes that need to happen in consumer code, so it should help you if you're trying to get a sense of what's going on in v0.2. Remember that it doesn't define the new functionality in the release, though. |
|
||
/** | ||
* Call the {@Chart#initialize} method up the inheritance chain, starting with | ||
* the base class and continuing "downward". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be "upward"? From the base up the chain? Just being a total jerk here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I don't know which term is technically correct here. My guess is that this is entirely subjective because the prototype chain has no physical analog. Here's my rationale for this phrasing: in my experience, the phrase "up the prototype chain," is used fairly often when people describe JavaScript's property access mechanism. That would make "down" the direction of base-to-instance.
I'm not satisfied using anecdotal evidence to decide on this, though. I want the documentation to make the most sense to the most people. Is there some more concrete reason to use the opposite wording?
Sent in a small pull req here #68. I'm rewriting the examples, but we'll merge those in a different pull req. |
Programatically wrap the concatenated source code in an immediately-invoked function expression at build time. This allows for easier code sharing between files, lessening the friction in dispersing the source across multiple files.
Now that the source files are programatically wrapped in an immediately-invoked function expression at build time, the base-line indentation should be removed.
This brings some great improvements! Can you give an estimate for the release date? If I understand the above correctly, you both consider the API changes to be final and only need to work on the website and docs. So it's reasonable to rely on API stability at this point? |
Lets get this out and about, I want to adopt it! :-D |
Hey everyone: thanks for your interest and encouragement! Irene and I just wrapped up a conference, so we finally have the time to release this version and support the transition process. We're shooting for early next week. In the mean time, feel free to experiment with version 0.2 today! |
Chart#extend and Chart#mixin are intended to help authors create new chart definitions from existing constructors. These new definitions may define their own data transformation logic, but they likely expect the previously-defined transformations to take place as well (which is why they have chosen to reuse the "base" chart).
Supporting a variable number of chart options needlessly complicates the internals of d3.chart.
In an effort to honor the single responsibility principle, modify `Chart#mixin` to operate with previously-created Chart instances. Additionally, require a unique string name for each mixin.
Provide mixin users with a means to split the input data across mixins.
The name "mixin" incorrectly implies state sharing, which this API has been designed to avoid. The name "attachment" more clearly communicates the intended use: to associate the `draw` function of one chart which contains another.
Because Layers emit a pre-defined set of event names, d3.chart can help developers prevent mistakes by asserting the name passed to `Layer#on` and `Layer#off` is part of that set.
If a lifecycle selection contains zero elements, do not trigger the associated lifecycle event handlers.
…e immediatly relevant than .
Ensure when charts are instantiated with a `transform` function, that function is properly attached to the chart instance. For example, the chart instance produced by the following code should contain the specified `transform` method: d3.chart("MyChart", { transform: function(d) { return d.attr; } });
Hi all, Version 0.2 is nigh, but I wanted to give you a heads-up regarding a The main reason is this: the limits of the functionality are too subjective. It would be a flaw for an API to solve only a subset of a problem. The API There are always dangers when expanding an existing API, but in light of the
Not All Doom and GloomThe feature request was initially motivated by the general problem with data This is the problem that d3.chart itself introduces, and it's the problem The new
...but it would be just as easy to include any arbitrary call to a data In other words, we've addressed the important problem without But I loved that FeatureWe try to be as open as possible about the development of this library. An At the same time, this release has plenty of other bug fixes and new features, To address these competing needs, we've extracted the Version 0.2 is stronger than ever, and we're gearing up to release it by the |
Increment version, rebuild distribution files, and update examples.
Hey everyone! I know it might not look it, but @iros and I have been hard at
work designing the next iteration of the library. We wanted to share our plans
with you and solicit feedback--let use know what you think!
Abstracted Data Attributes
Almost immediately after the release of d3.chart, developers identified an
aspect of reusable visualizations that we missed completely: the shape of the
input data. You can see #22 and #12 for discussion, but it boils down to this:
truely reusable charts shouldn't force users to define the input data in any
particular way.
The solution has been frought with gotchas, but I believe we're at a point
where we can support abstract data attributes. Chart authors would be required
to "reserve" the data that their chart relies on:
...and chart consumers could then (optionally) specify a "mapping" of the
chart's data shape to the shape of their own data:
d3.chart would preform this transformation in real-time to avoid the memory
overhead of copying potentially-large datasets wherever possible. To keep
d3.chart more focused, we're implementing this functionality in a standalone
JavaScript module: DataMap.
Named Mixins and Data Demuxing
Currently, the
Chart#mixin
method kind of does a lot. It creates a new chartwith the specified options and ties its
draw
function to that of the parentchart. We'd like to simplify the method to accept a name and a chart. This
means you will only have one canonical method for creating charts (
d3.chart
),and
Chart#mixin
simply creates associations between two charts'draw
methods.
"Great," you're thinking, "a more verbose API." I know, I know, but stick with
me--it's worth it!
By separating responsibilities like this, we can make it less confusing to wire
up charts in different ways.
Some charts don't need to be redrawn every time new data arrives (@iros and I
refer to these as "decorator" charts--a chart that defines a tooltip would be
one example). In this case, you would just make a chart, and not "mix it in".
Some charts explicitly want to share
draw
; often this is the case when youare combining two charts to visualize two datasets together. In that case, we
might have a "container" chart that mixes together a bar chart and a line
chart:
As you're accostumed to, the
TwoSets
chart'sdraw
method will invoke thedraw
method of bothBarChart
andLineChart
. The problem is that, withoutthe hacky
transform
trick (which we're currently using in one of our examplecharts,
unfortunately) both charts will be given the same data. I say, "unfortunately"
because this is not what's desired, normally. With named mixins, we can
concisely "demux" input data to more intuitively support this use case:
This lets you mix independent charts together more concisely.
Committing to Constructors
From the start, we've tried to implement a "d3-like" API to promote more
consistent code style for people accustomed to d3.js. This meant, among other
things, hiding away constructor functions and supporting more declarative forms
for instantiated charts ("look ma, no
new
!).Unfortunately, this approach makes it difficult to use module systems like AMD
or CommonJS (see #41). All along, you've been able to access chart
constructors, but their usage hasn't been documented or backed by tests. In
version 0.2, we'd like to formally define the behavior of the constructors
created by
d3.chart
andChart#extend
. This won't be an API change but aformal endorsement of an alternative way of handling Charts.
Simplified Internals
We want to make the internals a little more approachable. To do this, we plan
to remove the
variadicNew
function and instantiate Charts directly with thenew
operator. This will limit the API a bit, though: you won't be able towrite
initialize
methods with arbitrary arity--all charts will be limited toa single "options" parameter.
So what do you think? It's a lot of change, but we think (given a proper
migration guide), it will be worth it.