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

feat: add histogram mode #218

Merged
merged 43 commits into from Jun 11, 2019

Conversation

emmacunningham
Copy link
Contributor

@emmacunningham emmacunningham commented May 25, 2019

Summary

This PR introduces histogram mode which will ensure that bar series bars are aligned with ticks along the xDomain axis such that the tick is aligned with the starting edge of a bar instead of in the center as is the current only option for bar series bars.

This mode can be enabled in one of two ways:

  • using the enableHistogramMode prop on a BarSeriesSpec
  • using the HistogramBarSeriesSpec which is identical to the BarSeriesSpec except that the enableHistogramMode does not need to be specified

If a user enables histogram mode in either of these ways, any split or additional defined bar series will be rendered as stacked bars as histogram mode does not allow clustering of bars.

In the story below, we toggle enableHistogramMode on individual BarSeriesSpecs (there are two distinct specs in the story) and if either one has it enabled, all series will stack; likewise, when checking hasHistogramBarSeries will add a HistogramBarSeriesSpec, causing all of the series to stack.
histogram_stacking

Additional features

Things get a little bit weird in Histogram Mode when we add Line & Area series because the bucketed points need to align with something. We expose a prop (histogramModeAlignment with options start, center, end) on LineSeriesSpec, AreaSeriesSpec, and LineAnnotationSpec to allow the user to define how these points should align in Histogram Mode (this is not specified for RectAnnotationSpec because RectAnnotations operate more like bars / bands so they are always aligned relative to starting edge of the histogram bars):

histogram_alignment

We also add one more tick to the available ticks for the xAxis in histogram mode. This is because, when the ticks get shifted, we need to also mark the ending edge of the last bar in the series:

histogram_extra_tick

A note about implementation:

There are two ways to approach aligning the bars correctly:
(1) shift the position of the bars by xScale.bandwidth / 2

  • in this case, we also then need to shift the container by -xScale.band / 2 so that the first bar aligns with the starting edge of the chart and the last bar does not overflow the ending edge of the chart. we also need to shift the bars highlighting by xScale.bandwidth / 2.

(2) shift the position of the axis ticks by - xScale.bandwidth / 2

  • in this case, we also then need to shift the other chart elements by -xScale.bandwidth / 2 (line and area) so their points align correctly with the starting edge of a bar instead of the center. bars highlighting does not need to be shifted in this case.

The advantage of (1) is that it does not require touching axis tick positioning or other chart elements, which it might seem weird to adjust those positions for histogramMode which seems to be a very bar series specific concern. However (1) has a couple of disadvantages:
- the first axis tick may slightly overflow the starting edge of the chart
- we need xScale.bandwidth before it is available (xScale computation relies on chartDimensions so on first render, we won't have an offset value)

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

  • Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
  • This was checked for cross-browser compatibility, including a check against IE11
  • Proper documentation or storybook story was added for features that require explanation or tutorials
  • Unit tests were updated or added to match the most common scenarios
  • Each commit follows the convention

@emmacunningham emmacunningham added wip work in progress :chart Chart element related issue labels May 25, 2019
@codecov-io
Copy link

codecov-io commented May 29, 2019

Codecov Report

Merging #218 into master will increase coverage by 0.02%.
The diff coverage is 98.93%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #218      +/-   ##
==========================================
+ Coverage   97.82%   97.84%   +0.02%     
==========================================
  Files          36       36              
  Lines        2571     2647      +76     
  Branches      566      590      +24     
==========================================
+ Hits         2515     2590      +75     
  Misses         49       49              
- Partials        7        8       +1
Impacted Files Coverage Δ
src/specs/settings.tsx 100% <ø> (ø) ⬆️
src/lib/themes/light_theme.ts 100% <ø> (ø) ⬆️
src/lib/themes/dark_theme.ts 100% <ø> (ø) ⬆️
src/lib/themes/theme.ts 100% <ø> (ø) ⬆️
src/lib/utils/commons.ts 100% <ø> (ø) ⬆️
src/state/annotation_utils.ts 100% <100%> (ø) ⬆️
src/state/utils.ts 96.2% <100%> (+0.62%) ⬆️
src/lib/series/rendering.ts 97.94% <100%> (ø) ⬆️
src/lib/axes/axis_utils.ts 100% <100%> (ø) ⬆️
src/lib/series/specs.ts 100% <100%> (ø) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91dbcb6...bc6820f. Read the comment docs.

@emmacunningham emmacunningham marked this pull request as ready for review June 3, 2019 20:06
@emmacunningham emmacunningham removed the wip work in progress label Jun 3, 2019
axisScaleDomain: xDomain.domain,
axisScaleType: xDomain.scaleType,
axisScaleDomain: xDomain.domain, // is this used anymore?
axisScaleType: xDomain.scaleType, // is this used anymore?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think these are used anymore (and they are being hard coded always for the xDomain), is it ok to remove these?

@emmacunningham
Copy link
Contributor Author

emmacunningham commented Jun 6, 2019

Notes from our sync via zoom:

  • line & rect annotations will assume linear continuous scale in histogram mode (unaffected by alignment) (line annotation always assumes linear continuous scale; i.e. xDomain line annotation at 2 should pass through 2 on the x axis)
  • default to center alignment
  • in default BarSeriesSpec, rect tannotations should snap to the band edges (intermediary values will also "snap"). this will be the case for both linear and ordinal scales.

@emmacunningham
Copy link
Contributor Author

@markov00 Have updated per our discussion on Zoom in response to the items you mentioned in review:

(1) Line annotations will always align with the axis line and are unaffected by the HistogramModeAlignment:

histo_line

(2) Per our conversation, the default HistogramModeAlignment will be HistogramModeAlignments.Center. Otherwise the hover not taking into effect when the point is aligned to HistogramModeAlignments.End is left as is. As mentioned in our discussion, this is technically "correct" since on the other half of the point, that is no longer part to of the point's value even though the point is aligned to there.

(3) The RectAnnotation rendering has been improved so that:

in Histogram Mode, the RectAnnotation xDomain coordinates align with the axis, similar to LineAnnotation.

image

when not in Histogram Mode, if there is a bar series, then regardless of the type of scale, the xDomain coordinates will snap to the nearest tick values

(This is with the xDomain coordinates defined as x0: 0, x1: 0.5, note that it covers the area from 0 to 1.
image

when not in Histogram Mode, if there is only a line or bar series, then the xDomain coordinates will not snap and will align with the axis similar to how LineAnnotations always behave

image

Lastly, the validation for annotations has changed so that if we are in histogram mode with an additional tick, then the validation expands the end of the domain to domainEnd + scale.minInterval (see the annotation from 3 to 4):
image

@markov00 markov00 self-requested a review June 10, 2019 17:31
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emmacunningham I've checked again that PR. Everything is fine! I want only to suggest to change the default bar padding for the histogram mode to something relatively small like 0.05 or less. You can maybe introduce a a new histogramPadding value on the style and use that instead of barPadding.

@emmacunningham
Copy link
Contributor Author

emmacunningham commented Jun 10, 2019

@emmacunningham I've checked again that PR. Everything is fine! I want only to suggest to change the default bar padding for the histogram mode to something relatively small like 0.05 or less. You can maybe introduce a a new histogramPadding value on the style and use that instead of barPadding.

@markov00 👍 have added histogramPadding to Theme and set a default of 0.05.

@emmacunningham emmacunningham merged commit b418b67 into elastic:master Jun 11, 2019
markov00 pushed a commit that referenced this pull request Jun 11, 2019
# [5.1.0](v5.0.0...v5.1.0) (2019-06-11)

### Features

* add histogram mode ([#218](#218)) ([b418b67](b418b67))
@markov00
Copy link
Member

🎉 This PR is included in version 5.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Jun 11, 2019
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:chart Chart element related issue released Issue released publicly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants