From 7378cc78d9a17a85e6dcf5306c5cb919ba7686fe Mon Sep 17 00:00:00 2001 From: Scott Sheffield Date: Wed, 8 Jul 2020 16:54:52 -0400 Subject: [PATCH 1/5] add alignment prop for domain start and end label to keep within chart footprint --- src/XAxis.js | 1 + src/XAxisLabels.js | 20 ++++++++++++++++++-- src/utils/Axis.js | 2 ++ 3 files changed, 21 insertions(+), 2 deletions(-) diff --git a/src/XAxis.js b/src/XAxis.js index 5184ccbe..fabe8840 100644 --- a/src/XAxis.js +++ b/src/XAxis.js @@ -64,6 +64,7 @@ export default class XAxis extends React.Component { labelFormat: PropTypes.func, labelFormats: PropTypes.array, labels: PropTypes.array, + noLabelOverhang: PropTypes.bool, /** * Adds horizontal offset (along the XAxis) to the labels */ diff --git a/src/XAxisLabels.js b/src/XAxisLabels.js index 8784d3cf..aa3415c8 100644 --- a/src/XAxisLabels.js +++ b/src/XAxisLabels.js @@ -174,6 +174,12 @@ class XAxisLabels extends React.Component { * width - width of the given label */ labels: PropTypes.array, + /** + * Override default label positioning that center-aligns labels below their data point, which can lead to + * variable left spacing in typical charts. + * TODO: does not yet account for any right-aligned treatments, e.g. Y axis on the right side. + */ + noLabelOverhang: PropTypes.bool, /** * Round ticks to capture extent of given x domain from XYPlot. */ @@ -246,10 +252,14 @@ class XAxisLabels extends React.Component { const marginY = max( labels.map(label => Math.ceil(distance + label.height)), ); + let textAnchor = 'middle'; + if (propsWithDefaults.noLabelOverhang) { + textAnchor = 'start'; + } const [marginLeft, marginRight] = getLabelsXOverhang( xScale, labels, - 'middle', + textAnchor, ); return defaults( @@ -331,9 +341,15 @@ class XAxisLabels extends React.Component { ? bindTrailingArgs(callback, label.value) : null; }); + let textAnchor = 'middle'; + if (this.props.noLabelOverhang) { + if (i === 0) textAnchor = 'start'; + if (i === labels.length - 1 && xScale(xScale.domain()[1]) === x) + textAnchor = 'end'; + } const style = defaults( - { textAnchor: 'middle' }, + { textAnchor }, getValue(labelStyle, { x, y, ...label }, i), XAxisLabels.defaultProps.labelStyle, ); diff --git a/src/utils/Axis.js b/src/utils/Axis.js index 1217d9ee..9f04e840 100644 --- a/src/utils/Axis.js +++ b/src/utils/Axis.js @@ -31,6 +31,7 @@ export function getAxisChildProps(props) { labelFormats, labelOffset, labels, + noLabelOverhang, gridLineClassName, gridLineStyle, onMouseEnterLabel, @@ -88,6 +89,7 @@ export function getAxisChildProps(props) { labels, labelClassName, labelStyle, + noLabelOverhang, distance: labelDistance, format: labelFormat, formats: labelFormats, From 76a6442e382eac4614dbf151ab0db82c16b96d94 Mon Sep 17 00:00:00 2001 From: Scott Sheffield Date: Wed, 8 Jul 2020 17:09:00 -0400 Subject: [PATCH 2/5] updating a very rigidly defined unit test --- tests/jsdom/spec/utils.Axis.spec.js | 209 ++++++++++++++-------------- 1 file changed, 105 insertions(+), 104 deletions(-) diff --git a/tests/jsdom/spec/utils.Axis.spec.js b/tests/jsdom/spec/utils.Axis.spec.js index d5ae4839..6c018277 100644 --- a/tests/jsdom/spec/utils.Axis.spec.js +++ b/tests/jsdom/spec/utils.Axis.spec.js @@ -1,13 +1,13 @@ -import { expect } from "chai"; -import * as d3 from "d3"; -import _ from "lodash"; +import { expect } from 'chai'; +import * as d3 from 'd3'; +import _ from 'lodash'; import { getAxisChildProps, - getMouseAxisOptions -} from "../../../src/utils/Axis"; + getMouseAxisOptions, +} from '../../../src/utils/Axis'; -describe("Axis utils", () => { - it("getAxisChildProps", () => { +describe('Axis utils', () => { + it('getAxisChildProps', () => { const axisProps = { width: 400, height: 250, @@ -17,147 +17,148 @@ describe("Axis utils", () => { spacingBottom: 10, spacingLeft: 10, spacingRight: 10, - position: "top", - placement: "", + position: 'top', + placement: '', ticks: [10, 20, 30, 40, 50], tickCount: 5, tickLength: 8, - tickClassName: "my-ticks", - tickStyle: { stroke: "blue" }, - title: "what a title", + tickClassName: 'my-ticks', + tickStyle: { stroke: 'blue' }, + title: 'what a title', titleDistance: 12, - titleAlign: "center", + titleAlign: 'center', titleRotate: true, - titleStyle: { color: "blue" }, + titleStyle: { color: 'blue' }, labelDistance: 12, - labelClassName: "my-label", - labelStyle: { color: "brown" }, - labelFormat: "0a", - labelFormats: ["0a"], - labels: ["what a label"], - gridLineClassName: "my-grid", + labelClassName: 'my-label', + labelStyle: { color: 'brown' }, + labelFormat: '0a', + labelFormats: ['0a'], + labels: ['what a label'], + gridLineClassName: 'my-grid', labelOffset: 10, - gridLineStyle: { stroke: "blue" }, + gridLineStyle: { stroke: 'blue' }, onMouseEnterLabel: () => {}, onMouseMoveLabel: () => {}, onMouseLeaveLabel: () => {}, - onMouseClickLabel: () => {} + onMouseClickLabel: () => {}, }; const { ticksProps, gridProps, labelsProps, - titleProps + titleProps, } = getAxisChildProps(axisProps); expect(ticksProps).to.eql( _.pick(axisProps, [ - "width", - "height", - "xScale", - "yScale", - "ticks", - "tickCount", - "spacingTop", - "spacingBottom", - "spacingLeft", - "spacingRight", - "position", - "placement", - "tickLength", - "tickStyle", - "tickClassName" - ]) + 'width', + 'height', + 'xScale', + 'yScale', + 'ticks', + 'tickCount', + 'spacingTop', + 'spacingBottom', + 'spacingLeft', + 'spacingRight', + 'position', + 'placement', + 'tickLength', + 'tickStyle', + 'tickClassName', + ]), ); expect(gridProps).to.eql( Object.assign( {}, _.pick(axisProps, [ - "width", - "height", - "xScale", - "yScale", - "ticks", - "tickCount", - "spacingTop", - "spacingBottom", - "spacingLeft", - "spacingRight" + 'width', + 'height', + 'xScale', + 'yScale', + 'ticks', + 'tickCount', + 'spacingTop', + 'spacingBottom', + 'spacingLeft', + 'spacingRight', ]), { lineClassName: axisProps.gridLineClassName, - lineStyle: axisProps.gridLineStyle - } - ) + lineStyle: axisProps.gridLineStyle, + }, + ), ); expect(labelsProps).to.eql( Object.assign( - {}, + { noLabelOverhang: undefined }, _.pick(axisProps, [ - "width", - "height", - "xScale", - "yScale", - "ticks", - "tickCount", - "spacingTop", - "spacingBottom", - "spacingLeft", - "spacingRight", - "position", - "placement", - "labels", - "labelClassName", - "labelStyle", - "onMouseEnterLabel", - "onMouseMoveLabel", - "onMouseLeaveLabel", - "onMouseClickLabel" + 'width', + 'height', + 'xScale', + 'yScale', + 'ticks', + 'tickCount', + 'spacingTop', + 'spacingBottom', + 'spacingLeft', + 'spacingRight', + 'position', + 'placement', + 'labels', + 'labelClassName', + 'labelStyle', + 'noLabelOverhang', + 'onMouseEnterLabel', + 'onMouseMoveLabel', + 'onMouseLeaveLabel', + 'onMouseClickLabel', ]), { distance: axisProps.labelDistance, format: axisProps.labelFormat, formats: axisProps.labelFormats, - offset: axisProps.labelOffset - } - ) + offset: axisProps.labelOffset, + }, + ), ); expect(titleProps).to.eql( Object.assign( {}, _.pick(axisProps, [ - "width", - "height", - "position", - "placement", - "title", - "spacingTop", - "spacingBottom", - "spacingLeft", - "spacingRight" + 'width', + 'height', + 'position', + 'placement', + 'title', + 'spacingTop', + 'spacingBottom', + 'spacingLeft', + 'spacingRight', ]), { style: axisProps.titleStyle, distance: axisProps.titleDistance, alignment: axisProps.titleAlign, - rotate: axisProps.titleRotate - } - ) + rotate: axisProps.titleRotate, + }, + ), ); }); - describe("getMouseAxisOptions", () => { - it("throws error on invalid axis type", () => { + describe('getMouseAxisOptions', () => { + it('throws error on invalid axis type', () => { expect(() => { - getMouseAxisOptions("z", {}, {}); + getMouseAxisOptions('z', {}, {}); }).to.throw(Error); }); - it("returns valid mouse options for x axisType", () => { + it('returns valid mouse options for x axisType', () => { const mockEvent = { currentTarget: { getBoundingClientRect: () => { @@ -165,29 +166,29 @@ describe("Axis utils", () => { top: 0, left: 0, height: 300, - width: 300 + width: 300, }; - } + }, }, clientX: 50, - clientY: 0 + clientY: 0, }; const scale = d3 .scalePoint() - .domain(["a", "b", "c"]) + .domain(['a', 'b', 'c']) .range([0, 100]); - expect(getMouseAxisOptions("x", mockEvent, scale)).to.eql({ + expect(getMouseAxisOptions('x', mockEvent, scale)).to.eql({ event: mockEvent, outerX: 50, outerY: 0, xScale: scale, - xValue: "b" + xValue: 'b', }); }); - it("returns valid mouse options for y axisType", () => { + it('returns valid mouse options for y axisType', () => { const mockEvent = { currentTarget: { getBoundingClientRect: () => { @@ -195,25 +196,25 @@ describe("Axis utils", () => { top: 0, left: 0, height: 300, - width: 300 + width: 300, }; - } + }, }, clientX: 0, - clientY: 50 + clientY: 50, }; const scale = d3 .scalePoint() - .domain(["a", "b", "c"]) + .domain(['a', 'b', 'c']) .range([0, 100]); - expect(getMouseAxisOptions("y", mockEvent, scale)).to.eql({ + expect(getMouseAxisOptions('y', mockEvent, scale)).to.eql({ event: mockEvent, outerX: 0, outerY: 50, yScale: scale, - yValue: "b" + yValue: 'b', }); }); }); From f07c9884ea595a27b6ace3e9e2e57d42114aff03 Mon Sep 17 00:00:00 2001 From: Scott Sheffield Date: Wed, 8 Jul 2020 17:12:56 -0400 Subject: [PATCH 3/5] fix range lookup to actually use range directly --- src/XAxisLabels.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/XAxisLabels.js b/src/XAxisLabels.js index aa3415c8..c6b86607 100644 --- a/src/XAxisLabels.js +++ b/src/XAxisLabels.js @@ -344,7 +344,7 @@ class XAxisLabels extends React.Component { let textAnchor = 'middle'; if (this.props.noLabelOverhang) { if (i === 0) textAnchor = 'start'; - if (i === labels.length - 1 && xScale(xScale.domain()[1]) === x) + if (i === labels.length - 1 && xScale.range()[1] === x) textAnchor = 'end'; } From 74d64b716903467930f29a251dee773e9a1a174d Mon Sep 17 00:00:00 2001 From: Scott Sheffield Date: Thu, 9 Jul 2020 11:45:08 -0400 Subject: [PATCH 4/5] Updated comment on noLabelOverhang --- src/XAxisLabels.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/XAxisLabels.js b/src/XAxisLabels.js index c6b86607..78faa21f 100644 --- a/src/XAxisLabels.js +++ b/src/XAxisLabels.js @@ -176,8 +176,8 @@ class XAxisLabels extends React.Component { labels: PropTypes.array, /** * Override default label positioning that center-aligns labels below their data point, which can lead to - * variable left spacing in typical charts. - * TODO: does not yet account for any right-aligned treatments, e.g. Y axis on the right side. + * variable left spacing in typical charts, by making the points at the edges of the range align underneath + * the chart contents. */ noLabelOverhang: PropTypes.bool, /** From e999dfc5257b6797a4ac7b3400c5ce3bdcd26a3e Mon Sep 17 00:00:00 2001 From: Scott Sheffield Date: Thu, 9 Jul 2020 12:05:33 -0400 Subject: [PATCH 5/5] Updated comment on noLabelOverhang again --- src/XAxisLabels.js | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/XAxisLabels.js b/src/XAxisLabels.js index 78faa21f..3a39f6a3 100644 --- a/src/XAxisLabels.js +++ b/src/XAxisLabels.js @@ -175,9 +175,13 @@ class XAxisLabels extends React.Component { */ labels: PropTypes.array, /** - * Override default label positioning that center-aligns labels below their data point, which can lead to - * variable left spacing in typical charts, by making the points at the edges of the range align underneath - * the chart contents. + * Default label behavior places the text centered below the data point it delineates. This can allow + * overhang where the first and possibly last labels' text hangs over the edges of the x axis range. + * Setting this to `true` will force the first and last labels to align in such a way that their text does + * not exceed the x range. That is, the first label will be text-anchor: "start" instead of "middle", and + * the label marking the right edge of the chart will be anchored to the "end" instead of "middle". + * + * This affects spacing calculations. */ noLabelOverhang: PropTypes.bool, /**