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: enable or disable ruler #765

Merged
merged 11 commits into from
Sep 15, 2020
9 changes: 8 additions & 1 deletion packages/core/src/components/axes/ruler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,17 @@ export class Ruler extends Component {
domainValue: number;
originalData: any;
}[];
isRulerEnabled = Tools.getProperty(
Copy link
Member

Choose a reason for hiding this comment

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

this should go inside render, that way it's value will update with future renders

this.model.getOptions(),
"ruler",
"enabled"
);

render() {
this.drawBackdrop();
Copy link
Member

Choose a reason for hiding this comment

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

it seems like drawBackdrop is repeated in ruler & grid. Would ruler fully function without it?

Copy link
Contributor Author

@JennChao JennChao Aug 28, 2020

Choose a reason for hiding this comment

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

ruler needs backdrop since the event listener is applied on backdrop.
圖片

Copy link
Member

Choose a reason for hiding this comment

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

I get that ruler is dependant on backdrop, however I'm wondering if these lines are needed inside ruler.ts

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I am not sure about it. These lines of code have existed even before I added the ruler option.

Copy link
Member

Choose a reason for hiding this comment

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

Yes I'm just asking whether removing them on your end still makes everything work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As what I can see do far, removing code within line 253 to line 261 won't break any thing on my end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Codes within line 253 to line 261 have already been removed.

this.addBackdropEventListeners();
if (this.isRulerEnabled) {
Copy link
Member

Choose a reason for hiding this comment

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

what if ruler was enabled and now isn't?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

backdrop event listener will be remove first and then be created is if needed

this.addBackdropEventListeners();
}
Copy link
Member

Choose a reason for hiding this comment

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

so we remove & add event listeners every time? This doesn't sound healthy, there should be a better way to only remove or add when we need to

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since d3 doesn't have an api for checking whether a specific eventListener is already existed or not. Removing and then adding event listeners when needed is what we can do for now.

Copy link
Member

Choose a reason for hiding this comment

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

again this is the same situation as the other PR, we just need a boolean here to keep track of event listeners

Copy link
Contributor Author

@JennChao JennChao Sep 11, 2020

Choose a reason for hiding this comment

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

A flag, isEventListenerAdded, has been added.

}

formatTooltipData(tooltipData) {
Expand Down
11 changes: 11 additions & 0 deletions packages/core/src/configuration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
RadarChartOptions,
// Components
GridOptions,
RulerOptions,
AxesOptions,
TimeScaleOptions,
TooltipOptions,
Expand Down Expand Up @@ -69,6 +70,15 @@ export const grid: GridOptions = {
}
};

/**
* Ruler options
*/
export const ruler: RulerOptions = {
// enable or disable ruler
enabled: true
};


/**
* Tooltip options
*/
Expand Down Expand Up @@ -146,6 +156,7 @@ const axisChart: AxisChartOptions = Tools.merge({}, chart, {
axes,
timeScale,
grid,
ruler,
zoomBar: {
top: {
enabled: false,
Expand Down
8 changes: 8 additions & 0 deletions packages/core/src/interfaces/components.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,14 @@ export interface GridOptions {
};
}

/**
* Ruler options
*/
export interface RulerOptions {
enabled?: boolean;
}


export interface BarOptions {
width?: number;
maxWidth?: number;
Expand Down