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

EUI Barchart and Histogram refactoring #2

Merged
merged 68 commits into from
Jul 3, 2018
Merged

EUI Barchart and Histogram refactoring #2

merged 68 commits into from
Jul 3, 2018

Conversation

markov00
Copy link
Collaborator

@markov00 markov00 commented Jun 1, 2018

The PR introduce the following components:

  • EuiBarSeries for bar charts
  • EuiHistogramSeries for histograms
  • EuiAreaSeries for area charts
  • EuiLineSeriesfor line charts

The chart can be viewed in vertical (default) or horizontal mode.
A default set of axis is available, but developers can add their own configured axis.
Crosshair and Selection Brush tools are controlled and managed by XYChart.

General features

  • Vertical Crosshair
  • Horizontal Crosshair
  • Highlight on hover (@timroes is currently required on all type of chart? che crosshair is currently working across all series also on stacked charts)
  • callback when hover an element (used to highlight legend)
  • callback for bar clicks
  • props to highlight single series, single data point
  • Show/Hide Current Time marker
  • X and Y Axis
    • show/hide labels
    • filter labels (?)
    • rotate labels
    • truncate labels
    • positioning
    • default axis
    • custom ticks
    • set axis extent (clipping?)
    • scale data to bounds
    • horizontal grids
    • vertical grids
    • axis title below axis
    • test and examples for various scales
  • Legend
  • Multi-Axis charts
  • Brush selector (x, y, xy selections)
  • Allow users to configure fitting function for missing/null buckets Allow users to configure fitting/filling function for missing/null buckets elastic/kibana#17717
  • Color by series, by data
  • select interval with brush

Charts to be refactored (WIP)

  • Line chart
    • Single/multi line
    • Line mode
    • Show/Hide line in favour of scatterplot
    • Show/Hide line circles aka markers
    • Change line width
  • Scatterplot
  • Bar Chart
    • Vertical/horizontal (refactored into a single component)
    • Stacked
    • Clustered and stacked
  • Histogram Chart
    • Vertical/horizontal (refactored into a single component)
    • Stacked
  • Area Chart
    • Stacked
    • Line mode
    • Line and marks

General Todo:

  • Add more tests
  • Add stacked histogram example
  • Fix crosshair positioning
  • Add crosshair for horizontal charts
  • Review components naming and prefix with @mattapperson
  • Fix crosshair series name and time/data formatting
  • Optimize crosshair for performance
  • Force colors and styling
  • Add histogram data format specification into docs
  • Refactoring stackBy and cluster attributes (possibly using orientation prop)
  • Refactoring Vertical/Horizontal Series into a single Series controlled by orientation prop
  • Flip crosshair tooltip when crosshair along Y axis
  • Add scatterplot series

Discuss:

  • General implementation of data processing for histograms and missing values required for stacked charts (data needs an additional x0 or y0 values for the bin dimensioning)
    Notes: The data processing pipeline will take care of process data in the correct format.
    Some simple utility function can be built to support chart data format processing.
  • How to configure a chart for stacked/normal mode depending on used data series.
    Notes: It will not be automatically configured to stack series together. The developer needs to know when and how to stack data
  • Do we need to keep the crosshair active when brushing?
  • What is the behaviour of click on an area chart?

@markov00 markov00 mentioned this pull request Jun 1, 2018
8 tasks
@mattapperson mattapperson mentioned this pull request Jun 1, 2018
31 tasks
@markov00 markov00 changed the title WIP: EUI Barchart and Histogram refactoring EUI Barchart and Histogram refactoring Jun 29, 2018
@mattapperson mattapperson requested a review from sorenlouv June 29, 2018 14:55
title={title}
position={titlePosition}
orientation={orientation}
on0={onZero}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be onZero instead of on0?
Btw. onZero sounds more like an event handler than a boolean. Can't come up with a better name from the top of my head though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

on0 is how react-vis call it. We changed to onZero on our components. Maybe something like isOnZero sounds more like a bool

Copy link
Owner

Choose a reason for hiding this comment

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

agreed, isOnZero is a better name

Copy link
Collaborator

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

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

I've given this a quick look. On the surface it looks good - I don't have any objections. if there is anything in particular you want me to look at, let me know. Else next steps would be to try it out, and see how it works for APM data.

@mattapperson
Copy link
Owner

Still LGTM

@markov00 markov00 merged commit f380125 into mattapperson:chart Jul 3, 2018
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.

5 participants