Jamie/19 new line graph #3598

SEAjamieD · 2020-05-05T20:23:02Z

🔩 Description: What code changed, and why?

This is a new line graph being built for desktop. In its current state it is not reusable, but can be converted to usable in a swift manner when we are ready.

⛓️ Related Resources

fixes: https://github.com/chef/chef-design-system/issues/19
fixes #3723

👍 Definition of Done

Line graph is ready to be used in desktop and matches UX mock ups

👟 How to Build and Test the Change

build component/automate-ui-devproxy && start_all_services
run load_compliance_reports in hab studio
(unfortunately, I do not know how to provide varied data at this time)

navigate to https://a2-dev.test/desktop
Scroll down to "Last Check in By day"
See a chart with a few data points, change from 3 to 7 to 14 and all around using the dropdown within the component.
At 14, the labels will turn to allow for space.

Please note - the styles we are concerned about right now are only those of the graph, not its larger container.
Another important note - in its current container, 7 days worth of labels will overlap. In its planned implementation, the container will be around 600px, and the labels fit fine. Will work with UX to adjust as requested.

✅ Checklist

I have read the CONTRIBUTING document.
Tests added/updated?
Docs added/updated?
All commits have been signed-off for the Developer Certification of Origin.

📷 Screenshots, if applicable

it doesn't appear the GIF is running at full speed - so take with a grain of salt

SEAjamieD · 2020-05-05T21:29:42Z

components/automate-ui/src/app/modules/desktop/desktop.module.ts

@@ -25,20 +27,22 @@ import { DesktopRoutingModule } from './desktop-routing.module';
    ChefPipesModule
  ],
  exports: [


no major changes - just alphabetized them while I was here

SEAjamieD · 2020-05-05T21:30:22Z

...nents/automate-ui/src/app/page-components/simple-line-graph/simple-line-graph.component.html

+    <defs>
+      <linearGradient id="chef-gradient-purple" x1="0%" y1="0%" x2="100%" y2="0%" gradientTransform="rotate(315)">
+        <stop id="stop1" offset="0%" />
+        <stop id="stop2" offset="100%" />
+      </linearGradient>
+    </defs>


This is placed within the SVG so that we can reuse it as a gradient across any items needing to have our gradient

SEAjamieD · 2020-05-05T21:31:12Z

...nents/automate-ui/src/app/page-components/simple-line-graph/simple-line-graph.component.scss

+    background: linear-gradient(315deg, #3023AE, #C86DD7);
+
+    .inner {
+      color: white;


noticing a few of these and other colors that need variable treatment... will fix up

SEAjamieD · 2020-05-05T21:32:22Z

...ts/automate-ui/src/app/page-components/simple-line-graph/simple-line-graph.component.spec.ts

+
+  it('contains an svg area for the chart', () => {
+    expect(d3.select(component.svg)).not.toBeNull();
+  });


I really wanted to test the items getting appended to the graph, but I was having a difficult time selecting anything within the svg or outside of it that was being appended via d3. Any thoughts might be helpful. I was attempting to queryAll using By.css

SEAjamieD · 2020-05-05T21:33:12Z

components/automate-ui/src/app/page-components/simple-line-graph/simple-line-graph.component.ts

+  @Input() data: DayPercentage[] = [];
+  @Input() width = 900;
+  @Input() height = 156; // we want a 116px height on the ticks, this minus margins
+  private locked: number = null;


locked stores a number to reference when the graph is being redrawn or resized, so that it can re-attach to whatever the selected number was.

SEAjamieD · 2020-05-05T21:34:11Z

components/automate-ui/src/app/page-components/simple-line-graph/simple-line-graph.component.ts

+  get margin() {
+    return { right: 20, left: 20, top: 20, bottom: 20 };
+  }


While it seems like you would just want to say return 20, its important to store each of these individually, so that if you change just one number - like the right margin, the math is correct across the rest of the file.

SEAjamieD · 2020-05-05T21:34:51Z

components/automate-ui/src/app/page-components/simple-line-graph/simple-line-graph.component.ts

+  get domainY(): [number, number] {
+    const min = 0;
+    const max = 100; // since this based on a percentage we are doing 0 to 100;
+    return [min, max];
+  }


leaving these values as variables to make flexible in the future if we desire.

SEAjamieD · 2020-05-05T21:37:24Z

components/automate-ui/src/app/page-components/simple-line-graph/simple-line-graph.component.ts

+    const theLine = this.svgSelection.selectAll('.line').data([this.data], d => d.daysAgo);
+    theLine.exit().remove();
+    theLine.enter().append('path').attr('class', 'line').merge(theLine)
+      .transition().duration(1000)
+    .attr('d', line);
+  }


this is the d3 standard update pattern. You will see it a lot in this file. In order to update, this is needed, in this order.
It first searches for a selector that may or may not exist and passes it our data
Then it removes any of the elements from that search that are no longer needed.
Then it adds any new items from data its being passed and merges them.

SEAjamieD · 2020-05-05T21:38:48Z

components/automate-ui/src/app/page-components/simple-line-graph/simple-line-graph.component.ts

+
+  private renderPoints(): void {
+    const points = this.svgSelection.selectAll('circle.point')
+      .data(this.data, d => d.daysAgo);


you will notice that many of the d3 update patterns are passing a second parameter to .data(). in this case i'm referring to d => d.daysAgo. This provides d3 a key to index the data on, which allows us to keep a reference when we're animating the data around.

SEAjamieD · 2020-05-05T21:41:27Z

components/automate-ui/src/app/page-components/simple-line-graph/simple-line-graph.component.ts

+    // these numbers are specific to its container
+    const localWidth = this.width - this.margin.right - this.margin.left - 34;
+    const thisRange = [localWidth, -this.margin.right];
+    const thisScale = d3.scaleLinear()
+      .domain(this.domainX)
+      .range(thisRange);
+


the tooltips have been a bit difficult to work with. The reason is that we cannot append html elements to an SVG, instead we have to attach them somewhere else in the dom. If we attach to the body, the positioning is easier, but we have unintended scroll issues, i.e. a tooltip scrolls with us when it shouldnt.
Instead I've chosen to attach it to the svg parent container. The best thing I could come up with to make the scaling fit across the X axis is to adjust the scaling down to roughly what the svg container is.

SEAjamieD · 2020-05-05T21:42:05Z

components/automate-ui/src/app/page-components/simple-line-graph/simple-line-graph.component.ts

+    // these numbers are specific to its container
+    const thisRange = [this.width - 48, 53];
+    const thisScale = d3.scaleLinear()
+      .domain(this.domainX)
+      .range(thisRange);


I applied the same trick for the buttons for the same reason, however the given UX styles required this anyways.

SEAjamieD · 2020-05-05T21:43:29Z

components/automate-ui/src/app/page-components/simple-line-graph/simple-line-graph.component.ts

+    }
+  }
+
+  private renderGrid() {


All through rendering the grid is where you'll see the margin variables come into play. This is standard d3.

SEAjamieD · 2020-05-05T21:44:28Z

components/automate-ui/src/app/page-components/simple-line-graph/simple-line-graph.component.ts

+  private getHoveredElement(d3Event): number {
+    const classes = d3.select(d3Event.target).attr('class');
+    const match = classes.match(/elem-([0-9]{1,2})/g)[0];
+    const num = match.split('-')[1];
+    return num;
+  }


this function is used to gather which number element is being highlighted, which we can then use to highlight all of our other elements in pairing.

msorens

I realize you're not yet in review, but just a few comments.

...nents/automate-ui/src/app/page-components/simple-line-graph/simple-line-graph.component.scss

components/automate-ui/src/app/page-components/simple-line-graph/simple-line-graph.component.ts

SEAjamieD · 2020-05-11T19:11:50Z

Updated with stronger typing, named constants, and feedback from UI team

scottopherson · 2020-05-11T21:51:25Z

Fluid width behavior lookin good 👍

scottopherson · 2020-05-11T21:54:46Z

Tooltips seem to be a tad offset from center:

...nents/automate-ui/src/app/page-components/simple-line-graph/simple-line-graph.component.scss

components/automate-ui/src/app/modules/desktop/desktop.module.ts

...nents/automate-ui/src/app/page-components/simple-line-graph/simple-line-graph.component.html

...nents/automate-ui/src/app/page-components/simple-line-graph/simple-line-graph.component.scss

components/automate-ui/src/app/page-components/simple-line-graph/simple-line-graph.component.ts

scottopherson · 2020-05-11T23:01:08Z

...nents/automate-ui/src/app/page-components/simple-line-graph/simple-line-graph.component.html

@@ -0,0 +1,13 @@
+<svg #svg [attr.viewBox]="viewBox" 
+    [ngStyle]="{'width': width}" 
+    (window:resize)="onResize()">


This reminds me of a potential future issue with the compliance line chart as well. If the parent container of the chart should change its width for whatever reason (maybe an extra panel opens or something) then the chart will not resize itself because the overall viewport is not being resized:

However with the current dashboard layout and how it resizes its layout when opening side panels, the parent container of this chart still remains the same size so relying on window:resize is okay for now just like it's currently okay in the compliance reports view. But it could perhaps be an issue in the future should things change with the design so wanted to make a note of this elsewhere besides my own brain. It'll be trickier to implement but I know there are ways to avoid needing to manually redraw based on viewport or parent container width, giving us more flexible and performant charts to work with.

This is a really good snag. Thanks for documenting. I'll leave for now because of the reasons you mention, but will keep in mind. I'll do a little research on this when I have the opportunity.

oh interesting! 👍

ResizeObserver could offer a solution if a polyfill exists for IE.

SEAjamieD · 2020-05-12T00:30:14Z

Tooltips seem to be a tad offset from center:

Yeah - I've been having a rough time getting them lined up exact. I'll give it another look.

SEAjamieD · 2020-05-15T20:50:52Z

Waiting til desktop updates are merged to pull this one in

Signed-off-by: seajamied <[email protected]>

its gets a little unweildy with d3, and I can't seem to find good , solid resources on how to apply typescript to d3, so i'm leaving as is for now Signed-off-by: seajamied <[email protected]>

Signed-off-by: seajamied <[email protected]>

…wn container Signed-off-by: seajamied <[email protected]>

Signed-off-by: seajamied <[email protected]>

msorens

Looking good!
Two small things:

I am assuming that the name changed somewhere along the line? (Instructions said go to "Last Check in By day".)
The "100%" is partially obscured:

msorens · 2020-05-18T22:39:04Z

components/automate-ui/src/app/page-components/simple-line-graph/simple-line-graph.component.ts

+    if (this.data && this.data.length > 0) {
+      this.renderChart();
+    }


Wondering if you can do away with this by deleting the guard against changes.data.firstChange in ngOnChanges ?

seems like I can't - I always end up with errors in the console if I don't check for firstChange

SEAjamieD · 2020-05-19T17:28:37Z

@apriofrost - This is a simple change so can always update later, but wanted to make sure you've seen that whenever the last 2 check ins are in high percentages, the 100% gets obscured. Thanks for pointing out @msorens

SEAjamieD self-assigned this May 5, 2020

SEAjamieD added automate-ui ui labels May 5, 2020

SEAjamieD marked this pull request as ready for review May 5, 2020 21:02

SEAjamieD requested review from apriofrost and a team May 5, 2020 21:02

SEAjamieD commented May 5, 2020

View reviewed changes

SEAjamieD force-pushed the jamie/19-new-line-graph branch 2 times, most recently from 6ce896c to 5db3e71 Compare May 8, 2020 17:26

msorens reviewed May 9, 2020

View reviewed changes

SEAjamieD force-pushed the jamie/19-new-line-graph branch from a4ec8fd to 0862a29 Compare May 11, 2020 16:15

SEAjamieD requested review from msorens and a team May 11, 2020 19:11

scottopherson reviewed May 11, 2020

View reviewed changes

...nents/automate-ui/src/app/page-components/simple-line-graph/simple-line-graph.component.scss Show resolved Hide resolved

scottopherson reviewed May 11, 2020

View reviewed changes

SEAjamieD force-pushed the jamie/19-new-line-graph branch from 97f2e76 to 4f3fe2c Compare May 15, 2020 20:49

SEAjamieD added the DO-NOT-MERGE label May 15, 2020

seajamied added 20 commits May 18, 2020 12:08

Add basic testing, make functions private that should be

0be3480

Signed-off-by: seajamied <[email protected]>

Add as much typescript as I can

83e805f

its gets a little unweildy with d3, and I can't seem to find good , solid resources on how to apply typescript to d3, so i'm leaving as is for now Signed-off-by: seajamied <[email protected]>

Update transition, make percentage round on tooltip

495b4b9

Signed-off-by: seajamied <[email protected]>

Revert temp file change

95fe657

Signed-off-by: seajamied <[email protected]>

Alphabetize imports

b7c0264

Signed-off-by: seajamied <[email protected]>

Lint fixes

c161eef

Signed-off-by: seajamied <[email protected]>

move stop styles to scss

4f9c67a

Signed-off-by: seajamied <[email protected]>

lint fix

b53fffb

Signed-off-by: seajamied <[email protected]>

Create reusable transition and turn transition off for resize

14f7058

Signed-off-by: seajamied <[email protected]>

Keep lock circle in sync with transition, give label buttons their oo…

d8f9562

…wn container Signed-off-by: seajamied <[email protected]>

Update label spacing, render on init

53aa73b

Signed-off-by: seajamied <[email protected]>

Add more typescript return types

47e79fc

Signed-off-by: seajamied <[email protected]>

Fix type on locked

92fc9fa

Signed-off-by: seajamied <[email protected]>

Shorten transition time

fc30614

Signed-off-by: seajamied <[email protected]>

update css transitions to match

702c0f4

Signed-off-by: seajamied <[email protected]>

Stronger typing

2b7893d

Signed-off-by: seajamied <[email protected]>

Line strong typing

81b1faf

Signed-off-by: seajamied <[email protected]>

Update css to named variables

943e228

Signed-off-by: seajamied <[email protected]>

Fix all nits from scotts review

ddfcc7e

Signed-off-by: seajamied <[email protected]>

Little more cleanup in html

406cb9e

Signed-off-by: seajamied <[email protected]>

SEAjamieD force-pushed the jamie/19-new-line-graph branch from 4f3fe2c to 7bc0be3 Compare May 18, 2020 19:19

Merge conflict marker cleanup

99d2255

Signed-off-by: seajamied <[email protected]>

SEAjamieD force-pushed the jamie/19-new-line-graph branch from 7bc0be3 to 99d2255 Compare May 18, 2020 19:22

SEAjamieD removed the DO-NOT-MERGE label May 18, 2020

msorens approved these changes May 19, 2020

View reviewed changes

SEAjamieD merged commit c78f2df into master May 19, 2020

chef-expeditor bot deleted the jamie/19-new-line-graph branch May 19, 2020 17:34

This was referenced Oct 30, 2022

[Snyk] Security upgrade karma from 4.1.0 to 6.0.0 ekmixon/automate#74

Open

[Snyk] Security upgrade karma from 0.13.22 to 6.0.0 ekmixon/automate#75

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jamie/19 new line graph #3598

Jamie/19 new line graph #3598

SEAjamieD commented May 5, 2020 •

edited by susanev

Loading

SEAjamieD May 5, 2020

SEAjamieD May 5, 2020

SEAjamieD May 5, 2020

SEAjamieD May 5, 2020

SEAjamieD May 5, 2020

SEAjamieD May 5, 2020

SEAjamieD May 5, 2020

SEAjamieD May 5, 2020

SEAjamieD May 5, 2020

SEAjamieD May 5, 2020 •

edited

Loading

SEAjamieD May 5, 2020

SEAjamieD May 5, 2020

SEAjamieD May 5, 2020

msorens left a comment

SEAjamieD commented May 11, 2020

scottopherson commented May 11, 2020

scottopherson commented May 11, 2020

scottopherson May 11, 2020

SEAjamieD May 12, 2020

Shadae May 12, 2020

scottopherson May 14, 2020

SEAjamieD commented May 12, 2020

SEAjamieD commented May 15, 2020

msorens left a comment •

edited by SEAjamieD

Loading

msorens May 18, 2020

SEAjamieD May 19, 2020

SEAjamieD commented May 19, 2020

Jamie/19 new line graph #3598

Jamie/19 new line graph #3598

Conversation

SEAjamieD commented May 5, 2020 • edited by susanev Loading

🔩 Description: What code changed, and why?

⛓️ Related Resources

👍 Definition of Done

👟 How to Build and Test the Change

✅ Checklist

📷 Screenshots, if applicable

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SEAjamieD May 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msorens left a comment

Choose a reason for hiding this comment

SEAjamieD commented May 11, 2020

scottopherson commented May 11, 2020

scottopherson commented May 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SEAjamieD commented May 12, 2020

SEAjamieD commented May 15, 2020

msorens left a comment • edited by SEAjamieD Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SEAjamieD commented May 19, 2020

SEAjamieD commented May 5, 2020 •

edited by susanev

Loading

SEAjamieD May 5, 2020 •

edited

Loading

msorens left a comment •

edited by SEAjamieD

Loading