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

Marker Color is not applied for Lollipop Chart when we have data color inside the series #14103

Closed
manomanim opened this issue Aug 23, 2020 · 9 comments
Assignees

Comments

@manomanim
Copy link

Expected behaviour

Marker Color is not applied for Lollipop Chart when we have data color inside the series, but in area chart it is workning correctly

Actual behaviour

Marker Color is not applied for Lollipop Chart when we have data color inside the series, but in area chart it is workning correctly

Live demo with steps to reproduce

https://jsfiddle.net/dL53g7fp/1/

Product version

Highcharts

Affected browser(s)

ALL

@raf18seb
Copy link
Contributor

Hi @manomanim, thank you for reporting the issue.

The lollipop series inherits from the dumbbell series lowColor property which should be replaced by marker.fillColor.

@raf18seb raf18seb self-assigned this Aug 24, 2020
@bvaisvil
Copy link

Hiya @raf18seb, is there a workaround for this issue? It's a blocker for us.

@raf18seb
Copy link
Contributor

raf18seb commented Feb 16, 2021

Hi @bvaisvil, yes, here is a workaround: https://jsfiddle.net/BlackLabel/q74j3k5u/

(function(H) {
  var pick = H.pick,
    extend = H.extend;

  H.wrap(H.seriesTypes.lollipop.prototype.pointClass.prototype, 'setState', function(proceed) {
    proceed.apply(this, Array.prototype.slice.call(arguments, 1));

    var point = this,
      series = point.series,
      chart = series.chart,
      seriesLowColor = series.options.lowColor,
      seriesMarker = series.options.marker,
      pointOptions = point.options,
      pointLowColor = pointOptions.lowColor,
      zoneColor = point.zone && point.zone.color,
      lowerGraphicColor = pick(
        pointLowColor,
        seriesLowColor,
        pointOptions.marker ? pointOptions.marker.fillColor : null,
        seriesMarker ? seriesMarker.fillColor : null,
        pointOptions.color,
        zoneColor,
        point.color,
        series.color
      );

    if (!point.state && point.lowerGraphic && !chart.styledMode) {
      point.lowerGraphic.attr({
        fill: lowerGraphicColor
      });
    }
  });

  H.wrap(H.seriesTypes.lollipop.prototype, 'drawPoints', function(proceed) {
    proceed.apply(this, Array.prototype.slice.call(arguments, 1));

    var series = this,
      chart = series.chart,
      pointLength = series.points.length,
      i = 0,
      seriesLowColor = series.lowColor = series.options.lowColor,
      seriesMarkerOptions = series.options.marker,
      lowerGraphicColor,
      zoneColor,
      pointMarkerOptions;

    while (i < pointLength) {
      point = series.points[i];
      pointMarkerOptions = point.options.marker;
      if (point.lowerGraphic && !chart.styledMode) {
        zoneColor = point.zone && point.zone.color;
        lowerGraphicColor = pick(
          pointMarkerOptions ? pointMarkerOptions.fillColor : null,
          seriesMarkerOptions ? seriesMarkerOptions.fillColor : null,
          point.options.lowColor,
          seriesLowColor,
          point.options.color,
          zoneColor,
          point.color,
          series.color
        );
        point.lowerGraphic.attr({
          fill: lowerGraphicColor
        });
      }
      i++;
    }
  });
})(Highcharts)

@raf18seb raf18seb removed their assignment Feb 16, 2021
@bvaisvil
Copy link

@raf18seb Thank you very much!

@TorsteinHonsi
Copy link
Collaborator

Thanks! I have some architectural questions to the PR.

As far as I can see, the lollipop series is is similar to a scatter series, except

  • it draws a line back to the threshold, and
  • it has a default threshold of 0 like the area series, unlike the scatter series that has a null threshold by default.

With the current PR however, the dumbbell series is extending the area range series with methods to render the range like a dumbbell. Then the lollipop series is in turn extending the dumbbell to eliminate the dumbbells color logic in order to get it back to the original.

So the question is: does it make sense at all to have the lollipop inherit the dumbbell? Or should the lollipop instead inherit the Scatter series, have a default threshold of 0 and share the drawConnector and getConnectorAttribs functions with the dumbbell? Then the marker coloring would come for free directly from base Series.

@hubertkozik
Copy link
Member

Actually, yes, extending the Lollipop series as a child of Dumbbell in my PR seems like a bad idea. I think, that Rafał as the author of the Lollipop series knows some more detailed information about this. @raf18seb would you like to add your comment?

@raf18seb
Copy link
Contributor

raf18seb commented Sep 8, 2022

Sorry for the late reply!
I agree. I think we made a mistake a few years ago when we thought that a lollipop is similar to a column series (which obviously we cannot inherit from) - that's why we decided to have inheritance like arearange -> dumbbell -> lollipop.

@TorsteinHonsi would you like us to refactor this? @hubertkozik is happy to do that

@TorsteinHonsi
Copy link
Collaborator

@raf18seb @hubertkozik Yes, I think it should be refactored.

@hubertkozik
Copy link
Member

Closing as completed in 52a14dd.

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

Successfully merging a pull request may close this issue.

6 participants