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

Line chart reference Lines #422

Merged
merged 10 commits into from
Jun 19, 2017

Conversation

JoshSchoen
Copy link
Contributor

@JoshSchoen JoshSchoen commented Jun 5, 2017

What kind of change does this PR introduce? (check one with "x")

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior? (You can also link to an open issue here)
You are not able to add reference lines to the line charts.

What is the new behavior?
Ability to add up to 3 reference lines and range and labels to line charts

Demo Testing:

  • npm start
  • Change CHART TYPE to "Line Chart with Reference Lines"
  • Verify the reference lines appear along with the Min, Avg, Max labels
  • Click on DIMENSIONS and Fit Container
  • Verify the chart is in a responsive format and chart and legend are within the chart container
  • UNCHECK Fit Container
  • Click on OPTIONS
  • UNCHECK Reference Labels
  • Verify the references lines remain in the view and Max, Min, Avg, Max labels are removed from the view and the legend moves to the left
  • CHECK Reference Labels
  • UNCHECK Reference Lines
  • Verify the references lines, Min, Avg, Max labels are removed from the view and the legend moves to the left
  • CHECK Reference Lines
  • UNCHECK Show Y Axis
  • Verify the references lines, Min, Avg, Max labels, all ticks and Y axis labels are removed from the view and the legend moves to the left
  • CHECK Show Y Axis
  • UNCHECK Show Grid Lines
  • Verify the axis labels and the tick labels remain in the view and the grid lines, references lines, Min, Avg, Max labels are removed from the view and the legend moves to the left
  • CHECK Show Grid Lines
  • UNCHECK Show Legend
  • Verify the legend is removed and the chart covers the entire chart container
  • CHECK Show Legend
  • Remove the following from app.component.html
    [showRefLines]="showRefLines" [showRefLabels]="showRefLabels" [referenceLines]="refLines"
  • Verify there are NO reference lines and reference labels on the chart and no errors occur.

Reference line data testing:
This PR also supports 2 different types of reference lines for line charts single and a range.

To test this change [referenceLines]="refLines" to [referenceLines]="avgRefLine"

  • Repeat the steps above to verify there are no errors and the single reference lines is working the same.

Example of single reference line:
avgRefLine = { value: 37750, name: 'Average' };

Example of reference lines with a range:
refLines = [ { max: { value: 42500, name: 'Maximum' }, avg: { value: 37750, name: 'Average' }, min: { value: 33000, name: 'Minimum' } } ];

Does this PR introduce a breaking change? (check one with "x")

  • Yes
  • No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:
reference-lines-testing

@JoshSchoen
Copy link
Contributor Author

@marjan-georgiev just wanted to check to see if you had a chance to look at this. Let me know if there is anything that needs to be changed.

@marjan-georgiev
Copy link
Member

This is an awesome feature, but we can probably make it more generic, so it would support any number of reference lines, and would allow reference lines other than the min, avg, max ones defined here.

We could achieve this just by simplifying the refLines format from this:

refLines = [
   {
      max: { value: 42500, name: 'Maximum' },
      avg: { value: 37750, name: 'Average' },
      min: { value: 33000, name: 'Minimum' }
    }
];

to this:

refLines = [
  { value: 42500, name: 'Maximum' },
  { value: 37750, name: 'Average' },
  { value: 33000, name: 'Minimum' }
];

And in the y-axis-ticks component you can just iterate over this array and render all lines.

By doing that, I would now be able to define a custom reference line:

refLines = [
   { value: 3.14, name: 'PI' },
]

and would not be limited only to those 3.

What do you think?

@JoshSchoen
Copy link
Contributor Author

That makes sense. I'll make the changes. Thanks for the feedback!

@JoshSchoen
Copy link
Contributor Author

@marjan-georgiev updated the PR with the changes you mentioned. This ended up being a cleaner implementation so thanks for the suggestions. Let me know if you see anything else.

I thought about adding this to the bart charts as well but I think the reference line would have be implemented slightly different where the line would have to be in front of the bar to see it clearly. Could be a starting point to create bullet charts as well although that would have be on each bar series rather than across the entire chart. Let me know if you have any thoughts here.

@marjan-georgiev marjan-georgiev merged commit 31d4871 into swimlane:master Jun 19, 2017
@marjan-georgiev
Copy link
Member

Great, thanks! Let's discuss this for the bar chart in a separate issue.

@luizgabriellf
Copy link

Hello, an option to custom stroke color of theses limits?

@luizgabriellf
Copy link

@marjan-georgiev can reply my last comment? thanks

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.

None yet

4 participants