Skip to content

Commit

Permalink
Code review feedback
Browse files Browse the repository at this point in the history
Return the event directly, to speed up the code a bit.  Add JSDoc to help communicate its intent.  Update various comments.
  • Loading branch information
joshkel committed Feb 15, 2022
1 parent 0f99ce4 commit 77de6ef
Show file tree
Hide file tree
Showing 5 changed files with 28 additions and 15 deletions.
5 changes: 2 additions & 3 deletions src/core/core.controller.js
Expand Up @@ -794,9 +794,8 @@ class Chart {
}

/**
* Checks whether the given point is in the chart area. Point coordinates are
* relative to the chart. (getRelativePosition may be helpful.)
* @param {Point} point
* Checks whether the given point is in the chart area.
* @param {Point} point - in relative coordinates (see, e.g., getRelativePosition)
* @returns {boolean}
*/
isPointInArea(point) {
Expand Down
10 changes: 5 additions & 5 deletions src/core/core.interaction.js
Expand Up @@ -84,7 +84,7 @@ function getDistanceMetricForAxis(axis) {
/**
* Helper function to get the items that intersect the event position
* @param {Chart} chart - the chart
* @param {Point} position - the point to be nearest to
* @param {Point} position - the point to be nearest to, in relative coordinates
* @param {string} axis - the axis mode. x|y|xy|r
* @param {boolean} [useFinalPosition] - use the element's animation target instead of current position
* @return {InteractionItem[]} the nearest items
Expand All @@ -109,7 +109,7 @@ export function getIntersectItems(chart, position, axis, useFinalPosition) {
/**
* Helper function to get the items nearest to the event position for a radial chart
* @param {Chart} chart - the chart to look at elements from
* @param {Point} position - the point to be nearest to
* @param {Point} position - the point to be nearest to, in relative coordinates
* @param {string} axis - the axes along which to measure distance
* @param {boolean} [useFinalPosition] - use the element's animation target instead of current position
* @return {InteractionItem[]} the nearest items
Expand All @@ -133,7 +133,7 @@ function getNearestRadialItems(chart, position, axis, useFinalPosition) {
/**
* Helper function to get the items nearest to the event position for a cartesian chart
* @param {Chart} chart - the chart to look at elements from
* @param {Point} position - the point to be nearest to
* @param {Point} position - the point to be nearest to, in relative coordinates
* @param {string} axis - the axes along which to measure distance
* @param {boolean} [intersect] - if true, only consider items that intersect the position
* @param {boolean} [useFinalPosition] - use the element's animation target instead of current position
Expand Down Expand Up @@ -173,7 +173,7 @@ function getNearestCartesianItems(chart, position, axis, intersect, useFinalPosi
/**
* Helper function to get the items nearest to the event position considering all visible items in the chart
* @param {Chart} chart - the chart to look at elements from
* @param {Point} position - the point to be nearest to
* @param {Point} position - the point to be nearest to, in relative coordinates
* @param {string} axis - the axes along which to measure distance
* @param {boolean} [intersect] - if true, only consider items that intersect the position
* @param {boolean} [useFinalPosition] - use the element's animation target instead of current position
Expand All @@ -192,7 +192,7 @@ export function getNearestItems(chart, position, axis, intersect, useFinalPositi
/**
* Helper function to get the items matching along the given X or Y axis
* @param {Chart} chart - the chart to look at elements from
* @param {Point} position - the point to be nearest to
* @param {Point} position - the point to be nearest to, in relative coordinates
* @param {string} axis - the axis to match
* @param {boolean} [intersect] - if true, only consider items that intersect the position
* @param {boolean} [useFinalPosition] - use the element's animation target instead of current position
Expand Down
7 changes: 5 additions & 2 deletions src/helpers/helpers.canvas.js
Expand Up @@ -2,7 +2,10 @@ import {isArray, isNullOrUndef} from './helpers.core';
import {PI, TAU, HALF_PI, QUARTER_PI, TWO_THIRDS_PI, RAD_PER_DEG} from './helpers.math';

/**
* @typedef { import("../core/core.controller").default } Chart
* Note: typedefs are auto-exported, so use a made-up `canvas` namespace where
* necessary to avoid duplicates with `export * from './helpers`; see
* https://github.com/microsoft/TypeScript/issues/46011
* @typedef { import("../core/core.controller").default } canvas.Chart
* @typedef { import("../../types/index.esm").Point } Point
*/

Expand Down Expand Up @@ -95,7 +98,7 @@ export function _longestText(ctx, font, arrayOfThings, cache) {

/**
* Returns the aligned pixel value to avoid anti-aliasing blur
* @param {Chart} chart - The chart instance.
* @param {canvas.Chart} chart - The chart instance.
* @param {number} pixel - A pixel value.
* @param {number} width - The width of the element.
* @returns {number} The aligned pixel value.
Expand Down
19 changes: 15 additions & 4 deletions src/helpers/helpers.dom.js
@@ -1,5 +1,13 @@
import {INFINITY} from './helpers.math';

/**
* Note: typedefs are auto-exported, so use a made-up `dom` namespace where
* necessary to avoid duplicates with `export * from './helpers`; see
* https://github.com/microsoft/TypeScript/issues/46011
* @typedef { import("../core/core.controller").default } dom.Chart
* @typedef { import('../../types/index.esm').ChartEvent } ChartEvent
*/

/**
* @private
*/
Expand Down Expand Up @@ -83,12 +91,15 @@ function getCanvasPosition(e, canvas) {
return {x, y, box};
}

/**
* Gets an event's x, y coordinates, relative to the chart area
* @param {Event|ChartEvent} evt
* @param {dom.Chart} chart
* @returns {{x: number, y: number}}
*/
export function getRelativePosition(evt, chart) {
if ('native' in evt) {
return {
x: evt.x,
y: evt.y
};
return evt;
}

const {canvas, currentDevicePixelRatio} = chart;
Expand Down
2 changes: 1 addition & 1 deletion test/specs/helpers.dom.tests.js
Expand Up @@ -450,7 +450,7 @@ describe('DOM helpers tests', function() {
const nativePosition = Chart.helpers.getRelativePosition(nativeEvent, chart);

expect(chartPosition).not.toBeNull();
expect(nativePosition).toEqual(chartPosition);
expect(nativePosition).toEqual({x: chartPosition.x, y: chartPosition.y});
});
});
});

0 comments on commit 77de6ef

Please sign in to comment.