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(axis): option to hide duplicate axes #370

Merged
merged 7 commits into from
Sep 19, 2019
Merged

feat(axis): option to hide duplicate axes #370

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
37808ff
feat(axis): option to hide duplicate axes
nickofthyme Sep 10, 2019
e07b954
Merge branch 'master' into feat/remove-duplicate-axes
nickofthyme Sep 12, 2019
f306a93
fix: pr comments
nickofthyme Sep 12, 2019
add9661
test(chart_state): add tests for isDiplicateAxis function
nickofthyme Sep 12, 2019
37c4f0b
refactor: update reduce to map.filter
nickofthyme Sep 12, 2019
1f27b60
Merge branch 'master' into feat/remove-duplicate-axes
nickofthyme Sep 12, 2019
2474776
fix: pr comments
nickofthyme Sep 16, 2019
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
2 changes: 1 addition & 1 deletion src/chart_types/xy_chart/store/chart_state.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ export const isDuplicateAxis = (
) {
const spec = specMap.get(axisId);

if (spec && spec.position === position && ((!title && !spec.title) || title === spec.title)) {
if (spec && spec.position === position && title === spec.title) {
Copy link
Member

Choose a reason for hiding this comment

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

it's a double equal here if we want to mark as duplicate if one title is null and the other undefined (that in our case we can threat as no value)

Suggested change
if (spec && spec.position === position && title === spec.title) {
if (spec && spec.position === position && title == spec.title) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The value cannot be null based on the types correct?

Nonetheless, I also don't like double equals in javascript because it is loose equality, and generally a bad practice. It also can cause unwanted conversions producing inaccurate results. I'd like to enforce this throughout the code eventually.

I think the explicit equality adding twice the code to clearly indicate the comparison is worth it.

Copy link
Member

Choose a reason for hiding this comment

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

yep that value cannot be null, but I was thinking on the case the chart used in a JS world (people can do strange things there).
You are right about the loose equality, I use that only in rare case checking when checking for null and undefined, in that case you are right, we can get some strange results. So please ignore my comment here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To your point about people using this library in javaScript and not minding the types.

I 100% agree, but I think if we want to address that there are numerous places where we assume values to be provided/required. I think the Theme is a perfect example where we assume options are provided on the type were in js they may not.

hasDuplicate = true;
}
}
Expand Down
25 changes: 8 additions & 17 deletions src/components/react_canvas/reactive_chart.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { inject, observer } from 'mobx-react';
import { ContainerConfig } from 'konva';
import { Layer, Rect, Stage } from 'react-konva';

import { AnnotationId, AxisId } from '../../utils/ids';
import { AnnotationId } from '../../utils/ids';
import { isLineAnnotation, isRectAnnotation, AxisSpec } from '../../chart_types/xy_chart/utils/specs';
import { LineAnnotationStyle, RectAnnotationStyle, mergeGridLineConfigs } from '../../utils/themes/theme';
import {
Expand Down Expand Up @@ -37,8 +37,8 @@ interface ReactiveChartState {
};
}

interface AxisConfig {
id: AxisId;
interface AxisProps {
key: string;
axisSpec: AxisSpec;
axisTicksDimensions: AxisTicksDimensions;
axisPosition: Dimensions;
Expand Down Expand Up @@ -163,20 +163,20 @@ class Chart extends React.Component<ReactiveChartProps, ReactiveChartState> {
];
};

getAxes = (): AxisConfig[] => {
getAxes = (): AxisProps[] => {
const { axesVisibleTicks, axesSpecs, axesTicksDimensions, axesPositions } = this.props.chartStore!;
const ids = [...axesVisibleTicks.keys()];

return ids
.map((id) => ({
id,
key: `axis-${id}`,
ticks: axesVisibleTicks.get(id),
axisSpec: axesSpecs.get(id),
axisTicksDimensions: axesTicksDimensions.get(id),
axisPosition: axesPositions.get(id),
Copy link
Member

Choose a reason for hiding this comment

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

you can map this to something that respect the props of the <Axis> component like {key, ticks, axisSpec, axisTickDimensions, axisPosition }` so then you can just render it like

<Axis
   {...axis}
   chartTheme={chartTheme}
   debug={debug}
   chartDimensions={chartDimensions}
/>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

see 2474776

}))
.filter(
(config: Partial<AxisConfig>): config is AxisConfig => {
(config: Partial<AxisProps>): config is AxisProps => {
const { ticks, axisSpec, axisTicksDimensions, axisPosition } = config;

return Boolean(ticks && axisSpec && axisTicksDimensions && axisPosition);
Expand All @@ -188,17 +188,8 @@ class Chart extends React.Component<ReactiveChartProps, ReactiveChartState> {
const { chartTheme, debug, chartDimensions } = this.props.chartStore!;
const axes = this.getAxes();

return axes.map(({ id, ticks, axisSpec, axisTicksDimensions, axisPosition }) => (
<Axis
key={`axis-${id}`}
axisSpec={axisSpec}
axisTicksDimensions={axisTicksDimensions}
axisPosition={axisPosition}
ticks={ticks}
chartTheme={chartTheme}
debug={debug}
chartDimensions={chartDimensions}
/>
return axes.map(({ key, ...axisProps }) => (
<Axis {...axisProps} key={key} chartTheme={chartTheme} debug={debug} chartDimensions={chartDimensions} />
Copy link
Member

Choose a reason for hiding this comment

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

Have you extracted the key a bit more explicit on the code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry what do you mean by this?

I tried spreading the props with the key included and I get a linting error not adding the key to mapd child. Is that what you mean?

Copy link
Member

Choose a reason for hiding this comment

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

yes, sorry :(
strange that the ts linter doesn't know about that

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ya seems like they can get the keys of the object to verify

image

));
};

Expand Down