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

When there are more than 5 lines, line 6 an up are invisible #783

Closed
openjck opened this issue Dec 22, 2017 · 15 comments
Closed

When there are more than 5 lines, line 6 an up are invisible #783

openjck opened this issue Dec 22, 2017 · 15 comments
Milestone

Comments

@openjck
Copy link
Contributor

openjck commented Dec 22, 2017

Steps to reproduce

Download and open this HTML document

Actual result

The final 5 lines (in this case, the top-most 5 lines) are invisible. Hovering over them does cause a black circle to appear, however.

Expected result

All lines are colored. d3 has something that looks like it could help with this. As a last resort, if there are any lines that can't be assigned a color, having multiple black lines would be better than having insible lines.

@hamilton
Copy link
Collaborator

Agreed, that would be a better expected result. It should also simply have a larger set of colors.

@shikhar-scs
Copy link
Contributor

shikhar-scs commented Dec 23, 2017

However the README.md mentions something about this in the FAQ section.

I only see the first five lines in my chart, what gives?

The colors for the first five lines, areas and legends are defined in the stylesheet for 
the light and dark themes. For a sixth line, you would add the follow CSS rules:

.mg-line6-color {
    stroke: steelblue;
}

.mg-area6-color {
    fill: steelblue;
}

.mg-hover-line6-color {
    fill: steelblue;
}

.mg-line6-legend-color {
    color: steelblue;
}
If you're plotting more than five lines in the same chart and using color to encode 
some dimension of the data, then you probably need to rethink the chart.

So, I guess this has been done on purpose. However, taking the scope to around 10 different colors shouldn't cause much harm. I've found out where to work on this.

If allowed can I file a related PR please ? @hamilton whatsay

@wlach
Copy link
Collaborator

wlach commented Dec 23, 2017

My vote would be to allow up to 10 colors and render any additional series in black. I agree that most charts shouldn't need/have more than 5 different colors in them, but otoh I don't think it's helpful to be overly restrictive here given the low effort involved.

@shikhar-scs
Copy link
Contributor

@wlach similar thoughts I have.
Then maybe I'll pick up the rest of the 5 from the d3 series available here and file a PR soon.

@shikhar-scs
Copy link
Contributor

shikhar-scs commented Dec 24, 2017

@wlach I've started work recently and realised that actually it would be of much help if you could tell me the exact colours you would want apart from steelblue for 6. I just need to insert these specific colour codes into the file.

@hamilton
Copy link
Collaborator

hamilton commented Dec 24, 2017

This sounds like a great PR. I'd say:

1.) Extend to 10 colors. You can fill in the remaining 5 from the link you provided. Frankly the ordering is not particularly important past the 5 we have now (which were from category10 but moved around since we didn't want to see that light color as the second line all the time).
2.) Past 10, the lines should be black + they should have the appropriate classes in-place. This places the onus on the user to fill in the colors if necessary.
3.) we need to provide some documentation that this is how multi-line charts work, with an explanation about why we only support 10 colored lines.
4.) I'm on the fence about the usability of allowing colors for more than 10. On the one hand, if this were a piece of paper, I'd say more than 5 is a disaster probably. But we are on the DOM. Mouse interactions can help clarify a multi-line plots. My vote is usually to be more conservative, and this solution gives the user just enough rope at any rate.

@shikhar-scs
Copy link
Contributor

Ok @hamilton me right at work.

we need to provide some documentation that this is how multi-line charts work, with an explanation about why we only support 10 colored lines.

Maybe I'll file another one, improving the README for the same

Past 10, the lines should be black + they should have the appropriate classes in-place. This places the onus on the user to fill in the colors if necessary.

I'll keep this in mind for sure

@shikhar-scs
Copy link
Contributor

@hamilton
I had implemented the colors for the first 10 lines easily.
However, when I'm trying to implement this,

Past 10, the lines should be black + they should have the appropriate classes in-place. This places the onus on the user to fill in the colors if necessary.

I'm unable to identify the exact places in https://github.com/mozilla/metrics-graphics/blob/master/dist/metricsgraphics.js where the specific classes are being added to each path_line in the format mg-line${line_id}. I've already tried in the following places but in vain.

  function mg_default_color_for_path(this_path, line_id) {
    this_path.classed('mg-line' + (line_id) + '-color', true);
  }

&

if (args.color === null) {
          class_string += ' mg-line' + d.line_id + '-color';
        }
        return class_string;

      } else {
        class_string = 'mg-line' + d.line_id;
        if (args.color === null) class_string += ' mg-line' + d.line_id + '-color';
        return class_string;
      }
  • a couple of other places too. However, I'm not getting the expected results.

I've made a class called mg-line-black which should be added to the class string in case args.color is null and line_id>10

The colors that I've added are as shown below
colorsmetrics

Only if you could guide me a bit on where to make the exact changes for mg-line and legend also, it would complete the PR.

@hamilton
Copy link
Collaborator

hamilton commented Dec 25, 2017 via email

@shikhar-scs
Copy link
Contributor

Well that's no problem @hamilton I'll wait up till after the new year.

P.S. Merry Christmas 🎄 🎅

@shikhar-scs
Copy link
Contributor

So @hamilton if you are back, do you mind answering this

I'm unable to identify the exact places in https://github.com/mozilla/metrics-graphics/blob/master/dist/metricsgraphics.js where the specific classes are being added to each path_line in the format mg-line${line_id}.

@shikhar-scs
Copy link
Contributor

@hamilton are you back ?

@wlach
Copy link
Collaborator

wlach commented Jan 15, 2018

@shikhar-scs I can probably help on this a bit, though I'm less experienced with the code in question than @hamilton. Could you post your work-to-date in a PR (referencing this issue)? Then we can iterate on this together.

@wlach wlach added this to the v2.13 milestone Jan 15, 2018
@shikhar-scs
Copy link
Contributor

shikhar-scs commented Jan 16, 2018

@wlach yep Sure, I'll push my code rightaway 👍

EDIT : please have a look ta the PR filed by me below #791

@wlach
Copy link
Collaborator

wlach commented Jan 31, 2018

Hopefully we're good here now!

@wlach wlach closed this as completed Jan 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants