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

Markers not shown with areasplinerange plot #6736

Closed
drmrbrewer opened this Issue May 16, 2017 · 13 comments

Comments

Projects
None yet
3 participants
@drmrbrewer

drmrbrewer commented May 16, 2017

Expected behaviour

I would expect markers to be shown with an areasplinerange plot just as they are with an areaspline plot. Yes, the range aspect of it makes it a slightly different animal, having two values associated with each point rather than just one, but I would still expect it to be possible to show markers for those two points, instead of just nothing. When displaying a shared tooltip over a chart having a mix of areasplinerange and areaspline plot types, although values for both series appear in the tooltip, only the markers on the areaspline plot react to hover events.... it just doesn't look right IMHO... the range plot just looks "dead" in comparison to the animated non-range plot.

Actual behaviour

Markers are not shown with an areasplinerange plot.

Live demo with steps to reproduce

http://jsfiddle.net/L2uo2898/

Affected browser(s)

All

@pawelfus

This comment has been minimized.

Show comment
Hide comment
@pawelfus

pawelfus May 16, 2017

Contributor

Related UserVoice topic. It says for arearange but I assume it applies also for areasplinerange.

Contributor

pawelfus commented May 16, 2017

Related UserVoice topic. It says for arearange but I assume it applies also for areasplinerange.

@drmrbrewer

This comment has been minimized.

Show comment
Hide comment
@drmrbrewer

drmrbrewer May 16, 2017

Yes I assume so. Hopefully you will consider implementing this. I think it would be a very enhancing enhancement :)

drmrbrewer commented May 16, 2017

Yes I assume so. Hopefully you will consider implementing this. I think it would be a very enhancing enhancement :)

@pawelfus

This comment has been minimized.

Show comment
Hide comment
@pawelfus

pawelfus May 17, 2017

Contributor

Simple POC for both markers, which works like a plugin: http://jsfiddle.net/kezrmt9x/

  (function(H) {
    var origDrawPoints = H.Series.prototype.drawPoints,
      defaultOptions = H.getOptions().plotOptions,
      defaultMarkerOptions = defaultOptions.line.marker,
      defaultHaloOptions = defaultOptions.area.states;

    // Generate symbol:
    H.seriesTypes.arearange.prototype.getSymbol = function() {
      return this.getCyclic(
        'symbol',
        'circle',
        this.chart.options.symbols
      );
    };

    // Set options:
    H.wrap(H.seriesTypes.arearange.prototype, 'init', function(proceed, chart, userOptions) {
      var ret = proceed.apply(this, Array.prototype.slice.call(arguments, 1));

      this.options.marker = H.merge(defaultMarkerOptions, userOptions.marker || {});
      this.options.states = H.merge(defaultHaloOptions, userOptions.states || {});

      return ret;
    });

    // Render points:
    H.wrap(H.seriesTypes.arearange.prototype, 'drawPoints', function(proceed) {
      var len = this.points.length,
        point,
        i;

      // Draw bottom points:
      origDrawPoints.apply(this, Array.prototype.slice.call(arguments, 1));

      i = 0;
      while (i < len) {
        point = this.points[i];
        point.lowerGraphic = point.graphic;
        point.graphic = point.upperGraphic;
        point.plotY = point.plotHigh;
        i++;
      }

      // Draw top points:
      origDrawPoints.apply(this, Array.prototype.slice.call(arguments, 1));

      i = 0;
      while (i < len) {
        point = this.points[i];
        point.upperGraphic = point.graphic;
        point.graphic = point.lowerGraphic;
        point.plotY = point.plotLow;
        i++;
      }
    });

    // Update halo:
    H.wrap(H.seriesTypes.arearange.prototype.pointClass.prototype, 'haloPath', function(proceed) {
      var path = proceed.apply(this, Array.prototype.slice.call(arguments, 1));
      if (this.upperGraphic) {
        this.plotY = this.plotHigh;
        path = path.concat(
          proceed.apply(this, Array.prototype.slice.call(arguments, 1))
        );
        this.plotY = this.plotLow;
      }
      return path;
    });

    // Destroy upper graphic:
    H.wrap(H.seriesTypes.arearange.prototype.pointClass.prototype, 'destroy', function(proceed) {
      if (this.upperGraphic) {
        this.upperGraphic = this.upperGraphic.destroy();
      }

      return proceed.apply(this, Array.prototype.slice.call(arguments, 1));
    });
  })(Highcharts);

It's not a perfect solution (halo is clipped, there's no option to set different markers top vs bottom, etc.).

Contributor

pawelfus commented May 17, 2017

Simple POC for both markers, which works like a plugin: http://jsfiddle.net/kezrmt9x/

  (function(H) {
    var origDrawPoints = H.Series.prototype.drawPoints,
      defaultOptions = H.getOptions().plotOptions,
      defaultMarkerOptions = defaultOptions.line.marker,
      defaultHaloOptions = defaultOptions.area.states;

    // Generate symbol:
    H.seriesTypes.arearange.prototype.getSymbol = function() {
      return this.getCyclic(
        'symbol',
        'circle',
        this.chart.options.symbols
      );
    };

    // Set options:
    H.wrap(H.seriesTypes.arearange.prototype, 'init', function(proceed, chart, userOptions) {
      var ret = proceed.apply(this, Array.prototype.slice.call(arguments, 1));

      this.options.marker = H.merge(defaultMarkerOptions, userOptions.marker || {});
      this.options.states = H.merge(defaultHaloOptions, userOptions.states || {});

      return ret;
    });

    // Render points:
    H.wrap(H.seriesTypes.arearange.prototype, 'drawPoints', function(proceed) {
      var len = this.points.length,
        point,
        i;

      // Draw bottom points:
      origDrawPoints.apply(this, Array.prototype.slice.call(arguments, 1));

      i = 0;
      while (i < len) {
        point = this.points[i];
        point.lowerGraphic = point.graphic;
        point.graphic = point.upperGraphic;
        point.plotY = point.plotHigh;
        i++;
      }

      // Draw top points:
      origDrawPoints.apply(this, Array.prototype.slice.call(arguments, 1));

      i = 0;
      while (i < len) {
        point = this.points[i];
        point.upperGraphic = point.graphic;
        point.graphic = point.lowerGraphic;
        point.plotY = point.plotLow;
        i++;
      }
    });

    // Update halo:
    H.wrap(H.seriesTypes.arearange.prototype.pointClass.prototype, 'haloPath', function(proceed) {
      var path = proceed.apply(this, Array.prototype.slice.call(arguments, 1));
      if (this.upperGraphic) {
        this.plotY = this.plotHigh;
        path = path.concat(
          proceed.apply(this, Array.prototype.slice.call(arguments, 1))
        );
        this.plotY = this.plotLow;
      }
      return path;
    });

    // Destroy upper graphic:
    H.wrap(H.seriesTypes.arearange.prototype.pointClass.prototype, 'destroy', function(proceed) {
      if (this.upperGraphic) {
        this.upperGraphic = this.upperGraphic.destroy();
      }

      return proceed.apply(this, Array.prototype.slice.call(arguments, 1));
    });
  })(Highcharts);

It's not a perfect solution (halo is clipped, there's no option to set different markers top vs bottom, etc.).

@drmrbrewer

This comment has been minimized.

Show comment
Hide comment
@drmrbrewer

drmrbrewer May 17, 2017

@pawelfus nice work. I also note that, in my current design, I have points of a certain radius that expand on hover to a larger radius. With the new method of yours, this also isn't handled... see e.g. http://jsfiddle.net/ksubbk66/

Is there any reason why the marker property cannot be supported natively (and consistently) for the range type plot? Or maybe you're just working towards that?

drmrbrewer commented May 17, 2017

@pawelfus nice work. I also note that, in my current design, I have points of a certain radius that expand on hover to a larger radius. With the new method of yours, this also isn't handled... see e.g. http://jsfiddle.net/ksubbk66/

Is there any reason why the marker property cannot be supported natively (and consistently) for the range type plot? Or maybe you're just working towards that?

@pawelfus

This comment has been minimized.

Show comment
Hide comment
@pawelfus

pawelfus May 18, 2017

Contributor

Marker options is one of the etc's ;) Updated: http://jsfiddle.net/9n2927w8/ I agree, it would be cool to support natively this.

Contributor

pawelfus commented May 18, 2017

Marker options is one of the etc's ;) Updated: http://jsfiddle.net/9n2927w8/ I agree, it would be cool to support natively this.

@drmrbrewer

This comment has been minimized.

Show comment
Hide comment
@drmrbrewer

drmrbrewer May 18, 2017

@pawelfus I look forward to seeing this in the next version :)

drmrbrewer commented May 18, 2017

@pawelfus I look forward to seeing this in the next version :)

@pawelfus

This comment has been minimized.

Show comment
Hide comment
@pawelfus

pawelfus May 22, 2017

Contributor

Examples:

Contributor

pawelfus commented May 22, 2017

Examples:

@pawelfus

This comment has been minimized.

Show comment
Hide comment
@pawelfus

pawelfus May 22, 2017

Contributor

Internal note:

  • I'm not sure about official demo ( new look: http://jsfiddle.net/4c91m09w/ ) - probably we should disable markers or change them at least
  • it still needs to be added to the docs (I don't know where to add //docs reference)
Contributor

pawelfus commented May 22, 2017

Internal note:

  • I'm not sure about official demo ( new look: http://jsfiddle.net/4c91m09w/ ) - probably we should disable markers or change them at least
  • it still needs to be added to the docs (I don't know where to add //docs reference)
@drmrbrewer

This comment has been minimized.

Show comment
Hide comment
@drmrbrewer

drmrbrewer May 22, 2017

Could markers be disabled by default for range series, to maintain the existing behaviour (non breaking) but with the option to enable? I think the inclusion of markers on range series is a big improvement. Thanks.

drmrbrewer commented May 22, 2017

Could markers be disabled by default for range series, to maintain the existing behaviour (non breaking) but with the option to enable? I think the inclusion of markers on range series is a big improvement. Thanks.

@pawelfus

This comment has been minimized.

Show comment
Hide comment
@pawelfus

pawelfus May 22, 2017

Contributor

I will leave it up to @TorsteinHonsi I think it's questionable what's more important: consistency between series types for defaults or keeping old design. Probably if this could be released as part of 5.1.0 then it should be fine.

Contributor

pawelfus commented May 22, 2017

I will leave it up to @TorsteinHonsi I think it's questionable what's more important: consistency between series types for defaults or keeping old design. Probably if this could be released as part of 5.1.0 then it should be fine.

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi May 24, 2017

Collaborator

I prefer consistency between series types. I've simply disabled markers in one of the demos where I think it was disturbing.

Collaborator

TorsteinHonsi commented May 24, 2017

I prefer consistency between series types. I've simply disabled markers in one of the demos where I think it was disturbing.

@drmrbrewer

This comment has been minimized.

Show comment
Hide comment
@drmrbrewer

drmrbrewer Jun 1, 2017

Will this be in the next 5.0.x or does it wait until 5.1.0? Thanks.

drmrbrewer commented Jun 1, 2017

Will this be in the next 5.0.x or does it wait until 5.1.0? Thanks.

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Jun 8, 2017

Collaborator

It will be released with v5.0.13.

Collaborator

TorsteinHonsi commented Jun 8, 2017

It will be released with v5.0.13.

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