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

UI: line chart #4661

Merged
merged 14 commits into from
Sep 13, 2018
Merged

UI: line chart #4661

merged 14 commits into from
Sep 13, 2018

Conversation

DingoEatingFuzz
Copy link
Contributor

This is parter of a larger project to improve the cpu and memory usage charts in the UI

Just a couple line chart components.

Features

  1. Chart will always assume the dimensions of the container element, making this really flexible
  2. The y-axis ticks count is dynamic based on the height of the chart, but will always be an odd number to ensure a line at the midpoint.
  3. Charts resize automatically when the browser resizes.
  4. A tooltip in the style of the distribution bar tooltip
  5. Auto updates when data is added to the bound data property
  6. The tooltip also auto updates as data comes in and shifts the datum that's underneath the mouse
  7. A use-case specific stats chart that guarantees at least a five minute window instead of stretching 10 seconds of data across the entire width of the chart
  8. A hip gradient fill that has a "global" intensity instead of a "local" intensity (explained below).

image

Tooltip in action

image

Some smaller charts, with less y-axis ticks

image

A demonstration of the "global intensity" of the gradient. Notice that the chart on the right doesn't have as deep as blues near the line. This is because the intensity is mapped from the top to the bottom of the canvas instead of from the top to the bottom of the area under the line for a chart.

@DingoEatingFuzz DingoEatingFuzz requested a review from a team September 11, 2018 04:21
<g class="canvas {{chartClass}}">
<path class="line" d="{{line}}" />
<rect class="area" x="0" y="0" width="{{yAxisOffset}}" height="{{xAxisOffset}}" fill="url(#{{fillId}})" clip-path="url(#{{maskId}})" />
<rect class="hover-target" x="0" y="0" width="{{yAxisOffset}}" height="{{xAxisOffset}}" onhover={{action updateActiveDatum}} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Why both the onhover here and the mouseenter/move in the d3 binding in the JS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, oops. This onhover in the template should be deleted. Glimmer doesn't support binding events on svg elements; at least not in 2.18.

<rect class="hover-target" x="0" y="0" width="{{yAxisOffset}}" height="{{xAxisOffset}}" onhover={{action updateActiveDatum}} />
</g>
<g class="x-axis axis" transform="translate(0, {{xAxisOffset}})"></g>
<g class="y-axis axis" transform="translate({{yAxisOffset}}, 0)"></g>
Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering aloud about accessibility of the SVG and @madalynrose pointed me to https://developer.mozilla.org/en-US/docs/Web/SVG/Element/desc - may be worth adding here. And she found https://developer.paciellogroup.com/blog/2013/12/using-aria-enhance-svg-accessibility/ - check out the section under "ARIA enhanced SVG accessibility". Though I'm not sure what stats we'd put there... maybe a min and max?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh neat! Yeah, I'll look into this. The stat line charts, once they are in place, will also have related graphics and stats around them for the current value. I think those places will be the most useful for accessibility, but that doesn't mean svgs shouldn't have descriptions at least.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After playing with this for an hour or so I think the amount of work justifies its own PR. I'd still open it against the f-ui-improved-stats-chart branch, so it'd be done before the line chart hits master.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yeah that makes sense! Thanks for digging into it ❤️

chart.set('data', narrowData);

assert.equal(
+chart.get('xScale').domain()[0],
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just coercing to a Number?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. Comparing dates doesn't work due to them being passed by reference.

@armon
Copy link
Member

armon commented Sep 12, 2018

These look great 😍


yGridlines: computed('yScale', function() {
// The first gridline overlaps the x-axis, so remove it
const [, ...ticks] = this.get('yTicks');
Copy link
Contributor

Choose a reason for hiding this comment

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

👌

Copy link
Contributor

@meirish meirish left a comment

Choose a reason for hiding this comment

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

Looks good - I think you'll still want to remove that on hover before shipping it, but sounds like you might have work on top of this so your call!

@DingoEatingFuzz DingoEatingFuzz merged commit 5e2edf8 into f-ui-improved-stats-charts Sep 13, 2018
@DingoEatingFuzz DingoEatingFuzz deleted the f-ui-line-chart branch September 13, 2018 23:59
@DingoEatingFuzz DingoEatingFuzz mentioned this pull request Sep 26, 2018
5 tasks
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants