Skip to content
This repository was archived by the owner on Mar 14, 2024. It is now read-only.

web-sparkline-chart#1216

Merged
samthor merged 6 commits intov2from
v2+fix-sparkline
Aug 1, 2019
Merged

web-sparkline-chart#1216
samthor merged 6 commits intov2from
v2+fix-sparkline

Conversation

@samthor
Copy link
Contributor

@samthor samthor commented Jul 29, 2019

Get sparkline working in v2.

This is a reasonably big change and makes quality-of-life improvements over v1.

  • uses ResizeObserver (or fallback to resize) to resize the element
    • does not need scoreHeight and friends—reads size from element size
  • better failure cases (supports single point, no points)
  • does not show "Invalid Date" on poor data

Screen Shot 2019-07-29 at 13 31 27

@samthor samthor requested a review from robdodson July 29, 2019 03:35
@samthor samthor requested a review from mfriesenhahn as a code owner July 29, 2019 03:35
@googlebot googlebot added the cla: yes Contributor has signed the CLA label Jul 29, 2019
stroke-dashoffset: 1000;
animation: sparkline-dash-animation 300ms linear forwards;
stroke-linecap: round;
// TODO(robdodson): This doesn't animate even in the old site.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm cool just removing this stuff. I think Eric added it originally and my knowledge of SVG animation is nil :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

firstUpdated() {
this.cursorElement_ = this.renderRoot.querySelector("#cursor");
this.scoreElement_ = this.renderRoot.querySelector("#score");
this.announcerElement_ = this.querySelector(".sr-announcer");
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should turn this into an id as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as discussed in chat—no!

get fill() {
return this.hasAttribute("fill");
}
set demo(demo) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to keep this or remove it before we land?

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'm less worried since this isn't hundreds of kbs of data.

Copy link
Contributor

@robdodson robdodson left a comment

Choose a reason for hiding this comment

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

LGTM, just curious about follow up on a few comments.

@carolrivero carolrivero added this to the Sprint 2019-07-30 milestone Jul 30, 2019
@carolrivero carolrivero added the eng For items that require engineering work. label Jul 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes Contributor has signed the CLA eng For items that require engineering work.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants