Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: provide option to refresh range axis label #766

Merged
merged 9 commits into from
Oct 8, 2020
4 changes: 4 additions & 0 deletions packages/core/src/interfaces/axis-scales.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,10 @@ export interface AxisOptions {
* is axis visible or not
*/
visible?: boolean;
/**
* whether keep updating axis in real time while zoom domain is changing
*/
updateWhenZooming?: boolean;
}

/**
Expand Down
10 changes: 8 additions & 2 deletions packages/core/src/services/scales-cartesian.ts
Original file line number Diff line number Diff line change
Expand Up @@ -416,12 +416,18 @@ export class CartesianScales extends Service {
let allDataValues;
// If the scale is stacked
if (axisOptions.stacked) {
const dataValuesGroupedByKeys = this.model.getDataValuesGroupedByKeys();
const dataValuesGroupedByKeys = this.services.zoom.filterDataForRangeAxis(
this.model.getDataValuesGroupedByKeys(),
{ stacked: true }
);

allDataValues = dataValuesGroupedByKeys.map((dataValues) =>
sum(values(dataValues) as any)
);
} else {
allDataValues = displayData.map((datum) => datum[mapsTo]);
allDataValues = this.services.zoom
.filterDataForRangeAxis(displayData)
.map((datum) => datum[mapsTo]);
}

if (scaleType !== ScaleTypes.TIME && includeZero) {
Expand Down
70 changes: 69 additions & 1 deletion packages/core/src/services/zoom.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,47 @@
// Internal Imports
import { AxisPositions, Events, ScaleTypes } from "../interfaces";
import { Service } from "./service";
import { Events } from "../interfaces";
import { Tools } from "../tools";

// D3 imports
import { extent } from "d3-array";

export class Zoom extends Service {
isZoomBarEnabled() {
// CartesianScales service is only available in axis charts
if (!this.services.cartesianScales) {
return false;
}

// @todo - need to update this if zoom bar in other position (bottom, left, right) is supported
// check configuration
if (
!Tools.getProperty(
this.model.getOptions(),
"zoomBar",
"top",
"enabled"
)
) {
return false;
}

// @todo - Zoom Bar only supports main axis at BOTTOM axis and time scale for now
this.services.cartesianScales.findDomainAndRangeAxes(); // need to do this before getMainXAxisPosition()
const mainXAxisPosition = this.services.cartesianScales.getMainXAxisPosition();
const mainXScaleType = Tools.getProperty(
this.model.getOptions(),
"axes",
mainXAxisPosition,
"scaleType"
);

return (
mainXAxisPosition === AxisPositions.BOTTOM &&
mainXScaleType === ScaleTypes.TIME
);
}

// get display data for zoom bar
// basically it's sum of value grouped by time
getZoomBarData() {
Expand Down Expand Up @@ -80,4 +115,37 @@ export class Zoom extends Service {
"zoomRatio"
);
}

// filter out data not inside zoom domain
// to get better range value for axis label
filterDataForRangeAxis(displayData: object[], configs?: any) {
const zoomDomain = this.model.get("zoomDomain");
const mergedConfigs = Object.assign(
{ stacked: false }, // default configs
configs
);
const shouldUpdateRangeAxis = Tools.getProperty(
this.model.getOptions(),
"axes",
this.services.cartesianScales.getMainYAxisPosition(),
"updateWhenZooming"
Copy link
Member

Choose a reason for hiding this comment

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

I was just testing this and realized that updateWhenZooming is available for all axes, however it only takes effect for the main Y-axis. this means that I can set it to false for the main X-axis and it will do nothing

Copy link
Member

Choose a reason for hiding this comment

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

we should either just make it available for the range axis like before, or if we are allowing it to be used for all axis then it should achieve something when set to false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@theiliad The domain axis is always changing while zoom domain is changing, so no option is required.
This new option should only works for the range axis (main Y axis).
So I change it back to updateRangeAxis under zoomBar.

Updated in 2cd37ad.

Please let me know if this is not what you expected.

);
if (this.isZoomBarEnabled() && shouldUpdateRangeAxis && zoomDomain) {
const domainIdentifier = mergedConfigs.stacked
? "sharedStackKey"
: this.services.cartesianScales.getDomainIdentifier();
const filteredData = displayData.filter(
(datum) =>
new Date(datum[domainIdentifier]) >= zoomDomain[0] &&
new Date(datum[domainIdentifier]) <= zoomDomain[1]
);
// if no data in zoom domain, use all data to get full range value
// so only return filteredData if length > 0
if (filteredData.length > 0) {
return filteredData;
}
}
// return original data by default
return displayData;
}
}