Refactor median-chart #1199

hallieswan · 2023-06-16T22:03:49Z

Refactor median-chart to median-barchart to remove its dependency on dc.js
Move styles shared between median-barchart and score-barchart into global stylesheet

hallieswan · 2023-06-16T22:06:26Z

src/app/features/charts/components/median-barchart/median-barchart.component.html

+    #tooltip
+    (mouseout)="hideTooltip()"
+  ></div>
+  <svg #chart (window:resize)="onResize()"></svg>


Add an event binding so that the chart can be resized as the window is resized

hallieswan · 2023-06-16T22:18:59Z

src/app/features/charts/components/median-barchart/median-barchart.component.spec.ts

+    component.xAxisLabel = xAxisLabel;
+    component.yAxisLabel = yAxisLabel;
+
+    fixture.detectChanges();


The chart is created in ngOnChanges, which is called in unit tests after fixture.detectChanges is called the first time. Otherwise, ngOnChanges isn't called when setting an input manually. Move fixture.detectChanges into setUp method, so that the correct data for the test is set when ngOnChanges is called.

hallieswan · 2023-06-16T22:21:27Z

src/app/features/charts/components/median-barchart/median-barchart.component.spec.ts

+
+    fixture.detectChanges();
+    element = fixture.nativeElement;
+    const chart = element.querySelector('svg g');


Since the svg exists in the template and so may be present when the chart hasn't been created, use the first child added in createChart to check whether the chart exists

hallieswan · 2023-06-16T22:23:53Z

src/app/features/charts/components/median-barchart/median-barchart.component.spec.ts

+  it('should not render the chart if all values are negative', () => {
+    const { chart } = setUp(
+      SMALL_MOCK_DATA.map((obj) => {
+        return { ...obj, medianlogcpm: -1 * obj.medianlogcpm };
+      })
+    );
+
+    expect(element.querySelector('.chart-no-data')).toBeTruthy();
+
+    expect(chart).toBeFalsy();
+  });


Match the current behavior, which does not render the chart when all medianlogcpm values are negative

hallieswan · 2023-06-16T22:24:56Z

src/app/features/charts/components/median-barchart/median-barchart.component.spec.ts

+  it('should render the chart if there is positive data', () => {
+    const createChartSpy = spyOn(component, 'createChart').and.callThrough();
+    const { chart } = setUp();
+
+    expect(component.data?.length).toEqual(FULL_MOCK_DATA.length);
+    expect(createChartSpy).toHaveBeenCalled();
+    expect(chart).toBeTruthy();
+
+    expect(element.querySelector('.chart-no-data')).toBeFalsy();
+  });


Case where medianlogcpm values are positive --

develop feature

hallieswan · 2023-06-16T22:26:07Z

src/app/features/charts/components/median-barchart/median-barchart.component.spec.ts

+  it('should not render bars for negative values', () => {
+    const { chart } = setUp([
+      { ...SMALL_MOCK_DATA[0], medianlogcpm: -0.01 },
+      ...SMALL_MOCK_DATA.slice(1),
+    ]);
+
+    expect(chart?.querySelectorAll('rect').length).toEqual(
+      SMALL_MOCK_DATA.length - 1
+    );
+  });


Ensure that new implementation matches the current behavior, i.e. that negative medianlogcpm values are removed from the chart

hallieswan · 2023-06-16T22:29:47Z

src/app/features/charts/components/median-barchart/median-barchart.component.spec.ts

+  it('should render the meaningful expression threshold even when all values are small', () => {
+    const { chart } = setUp(SMALL_MOCK_DATA);
+
+    expect(
+      chart?.querySelector('.meaningful-expression-threshold-line')
+    ).toBeTruthy();
+  });


Currently, the meaningful expression threshold is only shown when the max medianlogcpm value is >= the threshold. In the refactored chart, the threshold is always shown, so that the user has a reference for whether the value is meaningful, even when the values are very small. Happy to change this to match the current behavior, if that's preferred though!

develop feature

This would be a good question for @JessterB

This sounds like an improvement to me!

I don't see this when running the branch locally, but that's fine.

hallieswan · 2023-06-16T22:30:30Z

src/app/features/charts/components/median-barchart/median-barchart.component.spec.ts

+  it('should alphabetize the x-axis values', () => {
+    setUp(SMALL_MOCK_DATA);
+
+    const sortedTissues = TISSUES.sort();
+    const xAxisTicks = element
+      .querySelector('svg g .x-axis')
+      ?.querySelectorAll('.tick');
+    xAxisTicks?.forEach((val, index) => {
+      expect(val.textContent).toEqual(sortedTissues[index]);
+    });
+  });


Ensure new implementation matches current behavior, which alphabetizes the x-axis values

hallieswan · 2023-06-16T22:32:53Z

src/app/features/charts/components/median-barchart/median-barchart.component.spec.ts

+  it('should not render the chart if there is no data', () => {
+    const { chart } = setUp([]);
+
+    expect(component.data?.length).toEqual(0);
+    expect(element.querySelector('.chart-no-data')).toBeTruthy();
+
+    expect(chart).toBeFalsy();
+  });


develop feature

hallieswan · 2023-06-16T22:33:50Z

src/app/features/charts/components/median-barchart/median-barchart.component.ts

+    this._data = data
+      .filter((el) => el.medianlogcpm && el.medianlogcpm > 0)


Remove negative medianlogcpm values

hallieswan · 2023-06-16T22:34:18Z

src/app/features/charts/components/median-barchart/median-barchart.component.ts

+  @Input() set data(data: MedianExpression[]) {
+    this._data = data
+      .filter((el) => el.medianlogcpm && el.medianlogcpm > 0)
+      .sort((a, b) => a.tissue.localeCompare(b.tissue));


And alphabetize the tissues, so the x-axis values are sorted

hallieswan · 2023-06-16T22:34:51Z

src/app/features/charts/components/median-barchart/median-barchart.component.ts

+    this.maxValueY = Math.max(
+      this.MEANINGFUL_EXPRESSION_THRESHOLD,
+      d3.max(this._data, (d) => d.medianlogcpm) || 0
+    );


Calculate a max value for the y-axis, so that the meaningful expression threshold is still shown if values are very small

hallieswan · 2023-06-16T22:37:02Z

src/app/features/charts/components/median-barchart/median-barchart.component.ts

+  // get the current width allotted to this chart or default
+  getChartBoundingWidth(): number {
+    return (
+      d3.select(this.chartRef.nativeElement).node()?.getBoundingClientRect()
+        .width || 500
+    );
+  }


Get the current width allotted to this chart, so that the width will update as the window is resized

As a suggestion, we can define 500 as a constant at the top of the code.

I think you might have moved a lot of the variables out for the scorebarchart component to the top. You could consider standardizing on that approach if you'd like.

Sure thing, done in the next commit

hallieswan · 2023-06-16T22:43:32Z

src/app/features/charts/components/median-barchart/median-barchart.component.ts

+        .append('rect')
+        .attr('class', 'medianbars')
+        .attr('x', (d) => xScale(d.tissue) as number)
+        .attr('y', (d) => yScale(d.medianlogcpm || 0))


Since the medianlogcpm field can be null, I needed to add || 0 (instead of d.medianlogcpm! which results in a lint warning: Forbidden non-null assertion). However, medianlogcpm shouldn't ever be null here, since we remove objects with null medianlogcpm values when setting _data. I handled the warning in a similar way throughout this component, but there's probably a better approach, so happy to adjust!

Just as a sanity check, we might want to ask @JessterB if the data might ever be null in MongoDB and if that would possibly have any meaning in the charts.

There are no null values in the data today. If there were nulls, I believe that we'd want to not plot them. That said, we have no plans to replace or update this data at this time. If we do replace the data someday, we should be able to remove any nulls during data processing anyway, so I wouldn't worry about it.

hallieswan · 2023-06-16T22:44:12Z

src/app/features/charts/components/median-barchart/median-barchart.component.ts

+        .append('g')
+        .attr('class', 'x-axis')
+        .attr('transform', `translate(0, ${innerHeight})`)
+        .call(xAxis.tickSizeOuter(0))


Remove outer x-axis ticks to more closely match the current implementation

I love your screenshots, do you think you could add before / after for @JessterB? I figure she might have some thoughts :)

Sure! Here are some screenshots demonstrating the different x-axis tick options:

without outer ticks* with outer ticks

*implementation currently in this PR

hallieswan · 2023-06-16T22:48:18Z

src/app/features/charts/components/median-barchart/median-barchart.component.ts

+  dimension: any;
+  group: any;
+
+  constructor(private helperService: HelperService) {}


A good portion of this component is pulled directly from score-barchart, so in the future, could be worthwhile to refactor the shared methods into a base chart component that each chart extends

hallieswan · 2023-06-16T23:02:04Z

src/app/features/charts/components/median-barchart/median-barchart.component.ts

+        .attr('y1', yScale(this.MEANINGFUL_EXPRESSION_THRESHOLD))
+        .attr('y2', yScale(this.MEANINGFUL_EXPRESSION_THRESHOLD))


Based on the chart description, I expected that the threshold line would always be shown at log2(5), which is ~2.3. However on develop, when the max value of the chart is close to the threshold value, the threshold value is drawn at slightly less than 2.

I used yScale to calculate the position for the threshold, so it is drawn at log2(5) in this case. However, I might've misunderstood how the threshold should be calculated, so happy to adjust as needed!

develop feature

Another great question for @JessterB. I haven't looked at this chart so I'm not 100% sure.

The threshold should be drawn at log2(5), which is what I see running this locally.

hallieswan · 2023-06-16T23:02:35Z

src/app/features/charts/components/median-barchart/median-barchart.component.ts

+  @HostListener('window:resize', ['$event'])
+  onResize() {
+    this.clearChart();
+    this.createChart();
+  }


Handles resizing the chart when the window is resized --

feature_resizeMedianChart.mov

Updated to use transitions when chart is resized on window resize to match the score-barchart transition:

median-chart-animation.mov

hallieswan · 2023-06-16T23:03:05Z

src/app/features/charts/components/score-barchart/score-barchart.component.html

@@ -1,7 +1,7 @@
-<div id="score-barchart">
-  <div id="score-barchart-tooltip" class="arrow-below tooltip-arrow" #tooltip (mouseout)="hideTooltip()"></div>
+<div id="score-barchart" class="chart-d3">


Allows this chart to use the shared styles

develop feature

hallieswan · 2023-06-16T23:03:31Z

src/app/features/charts/components/score-barchart/score-barchart.component.scss

@@ -1,84 +1,14 @@
-$tooltip-color: #63676C;


Moved into a new sass variable

hallieswan · 2023-06-16T23:04:00Z

src/app/features/charts/components/score-barchart/score-barchart.component.scss

-    position: relative;
-    height: 350px;
-
-    #score-barchart-tooltip {
-        position: absolute;
-        text-align: center;
-        padding: 5px;
-        font-size: 14px;
-        background-color: $tooltip-color;
-        color: white;
-        display: none;
-        z-index: 200;
-        opacity: 0.9;
-        width: 200px;
-        cursor: pointer;
-        border-radius: 5px;
-        pointer-events: none;
-    }
-
-    .tooltip-arrow {
-        &::before {
-            content: '';
-            position: absolute;
-            left: 50%;
-            border: 10px solid transparent;
-            transform: translateX(-50%);
-        }
-
-        &.arrow-below {
-            transform: translate(calc(-50%), calc(-100% - 20px));
-
-            &::before {
-                bottom: -9px;
-                border-bottom: 0;
-                border-top-color: $tooltip-color;
-            }
-        }
-    }
-


Moved into shared stylesheet - chart_d3.scss

hallieswan · 2023-06-16T23:04:15Z

src/app/features/charts/components/score-barchart/score-barchart.component.scss

-    .x-axis .tick {
-        cursor: default;
-    }
-
-    .y-axis .tick {
-        cursor: default;
-    }
-
-    text.x-axis-label {
-        cursor: default;
-    }
-
-    text.y-axis-label {
-        cursor: default;
-    }
-
-    .chart-no-data {
-        display: flex;
-        justify-content: center;
-        align-items: center;
-        height: 100%;
-    }


Moved into shared stylesheet

hallieswan · 2023-06-16T23:07:35Z

src/styles/components/_chart_d3.scss

+  svg {
+    width: 100%;
+  }


Ensure that the svg uses all available space, so that the chart's calculated width represents the total space this chart can take up and the chart will resize when the window is resized

hallieswan · 2023-06-16T23:08:11Z

src/styles/components/_chart_d3.scss

+    .chart-no-data-text {
+      font-size: var(--font-size-lg);
+      font-style: italic;
+      color: var(--color-gray-600);
+    }


Added styling previously added by _chart.scss

hallieswan · 2023-06-16T23:08:42Z

src/styles/components/_chart_d3.scss

+    &.arrow-above {
+      transform: translate(calc(-50%), calc(50%));
+
+      &::before {
+        top: -9px;
+        border-top: 0;
+        border-bottom-color: var(--color-tooltip);
+      }
+    }


Add styling previously added by _chart.scss

hallieswan · 2023-06-16T23:09:51Z

src/styles/components/_chart_d3.scss

+  position: relative;
+  height: 350px;


The majority of this styling was moved from score-barchart.scss, so that the styles could be shared with median-barchart. I'll comment on the styling that isn't pulled from score-barchart

hallieswan · 2023-06-16T23:15:06Z

src/app/features/charts/components/median-barchart/median-barchart.component.spec.ts

+  it('should show and hide tooltip', () => {
+    setUp();
+
+    const tooltip = element.querySelector<HTMLElement>('#tooltip');
+    expect(tooltip?.textContent).toBeFalsy();
+
+    const xAxisTick = element.querySelector('svg g .x-axis .tick');
+    const mouseEnterEvent = new MouseEvent('mouseenter', {
+      bubbles: true,
+      cancelable: true,
+    });
+    xAxisTick?.dispatchEvent(mouseEnterEvent);
+
+    expect(tooltip?.style.display).toEqual('block');
+    expect(tooltip?.textContent).toBeTruthy();
+
+    const mouseLeaveEvent = new MouseEvent('mouseleave', {
+      bubbles: true,
+      cancelable: true,
+    });
+    xAxisTick?.dispatchEvent(mouseLeaveEvent);
+
+    expect(tooltip?.style.display).toEqual('none');
+  });
+});


develop

develop_tooltips.mov

feature

feature_tooltips.mov

sagely1

Looks good to me. I haven't worked with this chart so some of the aesthetic/visual questions may be something Jess will have to weigh in on.

…between median-barchart and score-barchart into global stylesheet

hallieswan · 2023-07-07T21:30:06Z

src/app/features/charts/components/median-barchart/median-barchart.component.html

@@ -0,0 +1,12 @@
+<div id="median-barchart" #medianBarChartContainer class="chart-d3">


Add #medianBarChartContainer to get div size on window resize

hallieswan · 2023-07-07T21:31:45Z

src/app/features/charts/components/median-barchart/median-barchart.component.ts

+  private MIN_CHART_WIDTH = 500;
+  private CHART_HEIGHT = 350;


Move constants to top of the file

hallieswan · 2023-07-07T21:32:21Z

src/app/features/charts/components/median-barchart/median-barchart.component.ts

-    this.clearChart();
-    this.createChart();
-  }
+  resizeChart = (divSize: number): void => {


Use transitions when the chart is resized on window resize

hallieswan · 2023-07-07T21:32:33Z

src/styles/components/_chart_d3.scss

@@ -72,7 +72,7 @@
    }

    &.arrow-above {
-      transform: translate(calc(-50%), calc(50%));
+      transform: translate(calc(-50%), calc(50% - 12px));


Move tooltip closer to the x-axis label

…n threshold line when values are small

sagely1

Looks good!

JessterB

This looks really good! I don't see any problem with merging and releasing this.

hallieswan commented Jun 16, 2023

View reviewed changes

hallieswan marked this pull request as ready for review June 16, 2023 23:28

hallieswan requested a review from sagely1 June 16, 2023 23:28

hallieswan self-assigned this Jun 16, 2023

sagely1 approved these changes Jun 28, 2023

View reviewed changes

hallieswan added 2 commits July 7, 2023 10:52

refactor median-chart to remove dc.js dependency, move styles shared …

10cdbf0

…between median-barchart and score-barchart into global stylesheet

fix lint error

2096feb

hallieswan force-pushed the refactor-median-chart branch 2 times, most recently from d378ecb to bb5a078 Compare July 7, 2023 21:29

hallieswan commented Jul 7, 2023

View reviewed changes

hallieswan added 2 commits July 10, 2023 16:11

updates per PR and add transition on resize

832c530

match current chart behavior which does not show meaningful expressio…

8129a6d

…n threshold line when values are small

hallieswan force-pushed the refactor-median-chart branch from e52af66 to 8129a6d Compare July 10, 2023 23:15

hallieswan requested a review from sagely1 July 19, 2023 19:55

hallieswan added 2 commits February 12, 2024 13:47

merge develop, resolve conflicts in score-barchart.component.scss

428990c

AG-300: update json keys

8022bb9

sagely1 approved these changes Feb 13, 2024

View reviewed changes

sagely1 requested a review from JessterB February 15, 2024 23:00

JessterB approved these changes Mar 1, 2024

View reviewed changes

sagely1 merged commit 50a9b1f into Sage-Bionetworks:develop Mar 13, 2024
3 checks passed

		this._data = data
		.filter((el) => el.medianlogcpm && el.medianlogcpm > 0)

		.attr('y1', yScale(this.MEANINGFUL_EXPRESSION_THRESHOLD))
		.attr('y2', yScale(this.MEANINGFUL_EXPRESSION_THRESHOLD))

		@@ -0,0 +1,12 @@
		<div id="median-barchart" #medianBarChartContainer class="chart-d3">

Refactor median-chart #1199

Refactor median-chart #1199

Conversation

hallieswan commented Jun 16, 2023

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

hallieswan Jun 16, 2023 • 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

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

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

sagely1 Jun 28, 2023 • 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

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

hallieswan Jun 16, 2023 • 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

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

sagely1 left a 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

sagely1 left a comment

Choose a reason for hiding this comment

JessterB left a comment

Choose a reason for hiding this comment

hallieswan Jun 16, 2023 •

edited

Loading

sagely1 Jun 28, 2023 •

edited

Loading

hallieswan Jun 16, 2023 •

edited

Loading