Skip to content
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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

added bullet chart #923

wants to merge 1 commit into from

Conversation

espinielli
Copy link
Contributor

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.)

@mtraynham
Copy link
Contributor

Very, very cool. Three things:

  1. I wonder if we can make this work with the color mixin, so the colors aren't static, but user defined. I imagine d3.scale.quantize would be the proper option for this setup.
  2. I'd prefer if you revert the following line and get it working with jshint properly: https://github.com/espinielli/dc.js/commit/a621c4679c7c4e17430226cb386819e347603fc5#diff-35b4a816e0441e6a375cd925af50752cR64
  3. Is this a Crossfilter groupAll only chart, i.e. you wouldn't use dimensions?

@gordonwoodhull might ask you for some tests :)

@gordonwoodhull
Copy link
Contributor

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.

  1. Agree
  2. I think the reason for skipping jshint on that one file is that it is copied directly from the d3-plugin repo, which seems reasonable enough to me.
  3. I'd like to see the example actually use crossfilter rather than faking the .data() like this. It looks like this will work with regular groups, with a rather complex reduce function that creates ranges, measures, and markers.
  4. Yes, tests. I know, it's hard. But I can merge it a lot sooner (to 2.1) if I don't have to write the tests myself.

@mtraynham
Copy link
Contributor

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.

@gordonwoodhull
Copy link
Contributor

@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.

@gordonwoodhull
Copy link
Contributor

We also do use box.js from d3-plugins so maybe the submodule would be appropriate. I'm not sure.

@espinielli
Copy link
Contributor Author

git submodule does not have the granularity of a file.
Maybe git subtree is an option but I do not know much.

For now I just copied bullet.js and renamed to d3.bullet.js as was done similarly for box.js.
(Ok, plus the fix) So for easy of merging any newer version of it I avoided changing it due to jshint.

@gordonwoodhull gordonwoodhull modified the milestone: v2.1 Jul 30, 2015
@espinielli
Copy link
Contributor Author

Note that d3/d3-plugins#130 has been fixed in the meanwhile.

@SofiaGR
Copy link

SofiaGR commented Mar 28, 2017

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?

@gordonwoodhull
Copy link
Contributor

@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.

@SofiaGR
Copy link

SofiaGR commented Mar 28, 2017

@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.

@SofiaGR
Copy link

SofiaGR commented Apr 7, 2017

@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?

@gordonwoodhull
Copy link
Contributor

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.

@SofiaGR
Copy link

SofiaGR commented Apr 6, 2018

@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 :/

@gordonwoodhull
Copy link
Contributor

Ha, okay @SofiaGR, thanks for responding. Life is flux.

Please let us know if you find anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants