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.

Sign up for GitHub

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 scatter dot on charts except scatter chart #769

Merged
merged 7 commits into from
Sep 11, 2020
Merged

feat: enable or disable scatter dot on charts except scatter chart #769

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e434491
feat: enable or disable scatter dot on charts except scatter chart
JennChao Aug 27, 2020
f55700c
refactor: rename variable
JennChao Aug 28, 2020
68f0af7
Merge branch 'master' of https://github.com/JennChao/carbon-charts in…
JennChao Sep 2, 2020
d1aa973
Merge branch 'master' of https://github.com/JennChao/carbon-charts in…
JennChao Sep 3, 2020
77ebcf3
fix: always enable scatter dot for bubble chart
JennChao Sep 7, 2020
e26af21
Merge branch 'master' of https://github.com/JennChao/carbon-charts in…
JennChao Sep 11, 2020
a141dfc
refactor: have enabled option within points
JennChao Sep 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion packages/core/src/charts/scatter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,9 @@ export class ScatterChart extends AxisChart {
new TwoDimensionalAxes(this.model, this.services),
new Grid(this.model, this.services),
new Ruler(this.model, this.services),
new Scatter(this.model, this.services),
new Scatter(this.model, this.services, {
alwaysEnableScatterDot: true
}),
new Skeleton(this.model, this.services, {
skeleton: Skeletons.GRID
})
Expand Down
7 changes: 7 additions & 0 deletions packages/core/src/components/graphs/scatter-stacked.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,18 @@
// Internal Imports
import { Scatter } from "./scatter";
import { Roles } from "../../interfaces";
import { Tools } from "../../tools";

export class StackedScatter extends Scatter {
type = "scatter-stacked";

render(animate: boolean) {
const isScatterEnabled = Tools.getProperty(this.model.getOptions(), "scatterDotEnabled");
if (!this.configs.alwaysEnableScatterDot) {
if (!isScatterEnabled) {
return;
}
}
// Grab container SVG
const svg = this.getContainerSVG({ withinChartClip: true });

Expand Down
7 changes: 7 additions & 0 deletions packages/core/src/components/graphs/scatter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,13 @@ export class Scatter extends Component {
}

render(animate: boolean) {
const isScatterEnabled = Tools.getProperty(this.model.getOptions(), "scatterDotEnabled");
if (!this.configs.alwaysEnableScatterDot) {
if (!isScatterEnabled) {
return;
}
}

// Grab container SVG
const svg = this.getContainerSVG({ withinChartClip: true });

Expand Down
1 change: 1 addition & 0 deletions packages/core/src/configuration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ const axisChart: AxisChartOptions = Tools.merge({}, chart, {
axes,
timeScale,
grid,
scatterDotEnabled: true,
Copy link
Member

Choose a reason for hiding this comment

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

probably under the points flag as enabled?

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.

if we move scatterDotEnabled under the points flag as enabled, then only scatter chart can access the config option. However, axis charts, such as line chart and area chart, also need to access this config option as well.

Copy link
Member

Choose a reason for hiding this comment

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

Don't think this'd be the case. Any chart that uses scatter should extend scatter chart and their options should do the same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

圖片

Sometimes we don't want to have the scatter dot displayed. That's the reason why I setup this scatterDotEnabled option for letting the user enable or disable the scatter dot. And please note that, there will always be scatter dot displayed on scatter charts. Only line chart, area chart, and step chart can have the option to disable scatter dot.

Copy link
Member

Choose a reason for hiding this comment

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

Yes in the screenshot you have above, AreaChart should be extending scatter which means the enabled flag would be available in there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I need to figure out what you mean by saying extending scatter chart. Both area chart and scatter chart extend AxisChart and have scatter component in them. That's the reason why we can see the scatter dot. And from my understanding, scatter component will only be newed in AxisChart.
圖片

From what I have observed above, I think whether we want to show or not show the scatter dot, Scatter component, should not be configure in scatterChart option. scatterDotEnabled should be specify in axisChart option so all the axis charts with Scatter component can access this config option.

Copy link
Contributor

Choose a reason for hiding this comment

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

what @theiliad is saying is that the ScatterChartOptions extends the AxisChartOptions meaning that all charts that have Scatter dots will have ScatterChartOptions in addition to the AxisChartOptions.

so if you change ScatterChartOptions like this (charts.ts):

/**
 * options specific to scatter charts
 */
export interface ScatterChartOptions extends AxisChartOptions {
	/**
	 * options for the points
	 */
	points?: {
		/**
		 * sets the radius of the point
		 */
		radius: number;
		fillOpacity?: number;
		filled?: boolean;
		enabled?: boolean;
	};
}

than you can update how you access the config in both scatter-stacked.ts and scatter.ts:
const isScatterEnabled = Tools.getProperty(this.model.getOptions(), "points", "enabled");

zoomBar: {
top: {
enabled: false,
Expand Down
4 changes: 4 additions & 0 deletions packages/core/src/interfaces/charts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,10 @@ export interface AxisChartOptions extends BaseChartOptions {
* zoombar configuration
*/
zoomBar?: ZoomBarsOptions;
/**
* enable or disable scatter dot
*/
scatterDotEnabled?: boolean;
}

/**
Expand Down