-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
added bullet chart #923
base: develop
Are you sure you want to change the base?
added bullet chart #923
Conversation
Very, very cool. Three things:
@gordonwoodhull might ask you for some tests :) |
This is great. An entirely different approach from #692, both in terms of data and implementation. I like relying on d3.plugin rather than cramming this into a composite chart. I don't have a strong opinion about using more complex data items, as this PR does, versus using multiple data series as #692 does. They both have their advantages.
|
I have another idea for 2. I was originally thinking we add it as a bower dependency, but that idea fell over pretty quickly. d3/d3-plugins#122 As a second thought, can we just add it as a git-submodule instead of copying the code here directly. This seems much more appropriate for external code that we have decided to not maintain directly (jshint or otherwise). The issue reported in d3/d3-plugins#130 could be fixed with a PR and temporarily we just submodule the PR fork. |
@mtraynham, can you submodule just one file, or are you suggesting bringing in the whole repo for the one file? I've found submodules to interact in annoying ways with switching between branches, which I do all the time with git flow. Although technically correct it might be easier just to pull in the file. |
We also do use box.js from d3-plugins so maybe the submodule would be appropriate. I'm not sure. |
git submodule does not have the granularity of a file. For now I just copied |
Note that d3/d3-plugins#130 has been fixed in the meanwhile. |
So I added this chart to dc, but I'm not sure how to make it responsive. Everytime I try, the svg + bullet tends to return null...or I crash things. Any suggestions on how to make this responsive to window resize? |
@SofiaGR, thanks, that's excellent feedback. I really appreciate community review on all PRs. Currently there is not consistent support for resizing in dc.js charts, so I'm not surprised this contribution does not have the feature either. The first step to getting this fixed is to contribute a test for it. So far I have not automated the testing for resizing and there are instead an incomplete set of examples which you can use to test the feature on each chart by eye. Would you be willing to copy one of the examples and modify it to show and resize a bullet chart? You could open a PR with the test or just email it to me. |
@gordonwoodhull I'll look into it, I figured out the resize mostly, but need to see why the text is dangling around when the window resize occurs. |
@gordonwoodhull Ok, I have the bullet charts responsive in my app, but I had to make a few changes to the bullet-chart.js source-code for that. I also altered the d3.bullet.js merely to allow the user to choose the number of axisTicks since it was a request on my side (smaller screens meant that 10 axis ticks was too crowded). Plus I separated the title_orient from the bullet_orient. I wanted to keep the bullet chart thin in height but large in width while having the text switch around (bottom of bullet chart or left of it in my case) depending on screen size. Text is at the bottom if there's not enough space to show it at the left of bullet chart. By not enough space, I mean a width value that is actually adjusted to my bullet chart widths, so that is not in the source-code. How do you suggest I approach this now? |
Hi @SofiaGR, sorry I missed this! If you're still interested in contributing, you could start another PR based on this one plus your changes. dc.js 3.0 will be released soon with support for d3v4, and then we are porting some of these PRs to release them after that. |
@gordonwoodhull Aaah, 10 days later after I posted that, I started working for a new company. Not sure if I still have that code, need to check :/ |
Ha, okay @SofiaGR, thanks for responding. Life is flux. Please let us know if you find anything. |
Added a new chart: bullet chart, a must for dashboards!
See it live here
It is based on the bullet chart d3-plugin with a fix as described in the d3/d3-plugins#130
Note I explicitly ignored the "original" bullet chart file in the
lint
grunt task: this is code copied from another repo and should stay equal to the original so as to ease future inclusion of newer version (and so be able to see what has changed. Just my humble opinion.)