-
Notifications
You must be signed in to change notification settings - Fork 121
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: remove duplicate tick labels from axis #577
Changes from 15 commits
f784b2b
e71ac18
f4eafa5
f7a1aa0
a0c8839
d6fb645
f263fbf
d5abfd7
591c603
dc9a38f
efce9bf
60664b9
af7c506
935e64a
4976be7
1b76c69
5037110
1d888f7
ead2e9e
c5ac664
4e88447
054c088
55bf6ee
6ba6751
9aa1329
4952e6b
8a9739c
594c6f6
a63b9eb
9ed79f0
506bf0b
d0ff3c8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,7 +22,7 @@ import { AxisSpec, DomainRange, AxisStyle } from './specs'; | |
import { Position } from '../../../utils/commons'; | ||
import { LIGHT_THEME } from '../../../utils/themes/light_theme'; | ||
import { AxisId, GroupId } from '../../../utils/ids'; | ||
import { ScaleType } from '../../../scales'; | ||
import { ScaleType, Scale } from '../../../scales'; | ||
import { | ||
AxisTick, | ||
AxisTicksDimensions, | ||
|
@@ -48,13 +48,16 @@ import { | |
getAxisTickLabelPadding, | ||
isVerticalGrid, | ||
isHorizontalGrid, | ||
getDuplicateTicks, | ||
} from './axis_utils'; | ||
import { CanvasTextBBoxCalculator } from '../../../utils/bbox/canvas_text_bbox_calculator'; | ||
import { SvgTextBBoxCalculator } from '../../../utils/bbox/svg_text_bbox_calculator'; | ||
import { niceTimeFormatter } from '../../../utils/data/formatters'; | ||
import { mergeYCustomDomainsByGroupId } from '../state/selectors/merge_y_custom_domains'; | ||
import { ChartTypes } from '../..'; | ||
import { SpecTypes } from '../../../specs/settings'; | ||
import { DateTime } from 'luxon'; | ||
import { computeXScale } from './scales'; | ||
|
||
describe('Axis computational utils', () => { | ||
const mockedRect = { | ||
|
@@ -202,7 +205,7 @@ describe('Axis computational utils', () => { | |
expect(axisDimensions).toEqual(axis1Dims); | ||
|
||
const computeScalelessSpec = () => { | ||
computeAxisTicksDimensions(ungroupedAxisSpec, xDomain, [yDomain], 1, bboxCalculator, 0, axes); | ||
computeAxisTicksDimensions(ungroupedAxisSpec, xDomain, [yDomain], 1, bboxCalculator, 0, axes, undefined, false); | ||
}; | ||
|
||
const ungroupedAxisSpec = { ...verticalAxisSpec, groupId: 'foo' }; | ||
|
@@ -1436,4 +1439,125 @@ describe('Axis computational utils', () => { | |
|
||
expect(getAxisTickLabelPadding(axisConfigTickLabelPadding, axisSpecStyle)).toEqual(2); | ||
}); | ||
test('should show unique tick labels if duplicateTicks is set to false', () => { | ||
const now = DateTime.fromISO('2019-01-11T00:00:00.000') | ||
.setZone('utc+1') | ||
.toMillis(); | ||
const oneDay = 1000 * 60 * 60 * 24; | ||
const formatter = niceTimeFormatter([now, now + oneDay * 31]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Better yet you could use moment's super easy and declarative API for this using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changes in commit 55bf6ee thanks! |
||
const axisSpec: AxisSpec = { | ||
id: 'bottom', | ||
position: 'bottom', | ||
duplicateTicks: false, | ||
chartType: 'xy_axis', | ||
specType: 'axis', | ||
groupId: '__global__', | ||
hide: false, | ||
showOverlappingLabels: false, | ||
showOverlappingTicks: false, | ||
tickSize: 10, | ||
tickPadding: 10, | ||
tickLabelRotation: 0, | ||
tickFormat: formatter, | ||
}; | ||
const xDomainTime: XDomain = { | ||
type: 'xDomain', | ||
isBandScale: false, | ||
domain: [1547190000000, 1547622000000], | ||
minInterval: 86400000, | ||
scaleType: ScaleType.Time, | ||
}; | ||
const scale: Scale = computeXScale({ xDomain: xDomainTime, totalBarsInCluster: 0, range: [0, 603.5] }); | ||
// xDomain: { type: 'xDomain', scaleType: 'time', isBandScale: false }, | ||
// | ||
// minInterval: | ||
// timeZone: undefined, | ||
// totalBarsInCluster: 0, | ||
// range: [0, 603.5], | ||
// barsPadding: 0.25, | ||
// enableHistogramMode: false, | ||
// ticks: undefined, | ||
// integersOnly: undefined, | ||
// duplicateTicks: false, | ||
// = { | ||
// domain: [], | ||
// barsPadding: 0.25, | ||
// bandwidth: 0, | ||
// type: 'time', | ||
// range: [0, 603.5], | ||
// minInterval: 86400000, | ||
// isInverted: false, | ||
// tickValues: [ | ||
// 1547208000000, | ||
// 1547251200000, | ||
// 1547294400000, | ||
// 1547337600000, | ||
// 1547380800000, | ||
// 1547424000000, | ||
// 1547467200000, | ||
// 1547510400000, | ||
// 1547553600000, | ||
// 1547596800000, | ||
// ], | ||
// isSingleValue: () => false, | ||
// totalBarsInCluster: 0, | ||
// bandwidthPadding: 0, | ||
// step: 0, | ||
// timeZone: 'utc', | ||
// isSingleValueHistogram: false, | ||
// }; | ||
const offset = 0; | ||
const tickFormatOption = { timeZone: undefined }; | ||
expect(getDuplicateTicks(axisSpec, scale, offset, tickFormatOption)).toEqual([ | ||
{ value: 1547208000000, label: '2019-01-11', position: 25.145833333333332 }, | ||
{ value: 1547294400000, label: '2019-01-12', position: 145.84583333333333 }, | ||
{ value: 1547380800000, label: '2019-01-13', position: 266.54583333333335 }, | ||
{ value: 1547467200000, label: '2019-01-14', position: 387.24583333333334 }, | ||
{ value: 1547553600000, label: '2019-01-15', position: 507.9458333333333 }, | ||
]); | ||
}); | ||
test('should show duplicate tick labels if duplicateTicks is set to true', () => { | ||
const now = DateTime.fromISO('2019-01-11T00:00:00.000') | ||
.setZone('utc+1') | ||
.toMillis(); | ||
const oneDay = 1000 * 60 * 60 * 24; | ||
const formatter = niceTimeFormatter([now, now + oneDay * 31]); | ||
const axisSpec: AxisSpec = { | ||
id: 'bottom', | ||
position: 'bottom', | ||
duplicateTicks: true, | ||
chartType: 'xy_axis', | ||
specType: 'axis', | ||
groupId: '__global__', | ||
hide: false, | ||
showOverlappingLabels: false, | ||
showOverlappingTicks: false, | ||
tickSize: 10, | ||
tickPadding: 10, | ||
tickLabelRotation: 0, | ||
tickFormat: formatter, | ||
}; | ||
const xDomainTime: XDomain = { | ||
type: 'xDomain', | ||
isBandScale: false, | ||
domain: [1547190000000, 1547622000000], | ||
minInterval: 86400000, | ||
scaleType: ScaleType.Time, | ||
}; | ||
const scale: Scale = computeXScale({ xDomain: xDomainTime, totalBarsInCluster: 0, range: [0, 603.5] }); | ||
const offset = 0; | ||
const tickFormatOption = { timeZone: undefined }; | ||
expect(getDuplicateTicks(axisSpec, scale, offset, tickFormatOption)).toEqual([ | ||
{ value: 1547208000000, label: '2019-01-11', position: 25.145833333333332 }, | ||
{ value: 1547251200000, label: '2019-01-11', position: 85.49583333333334 }, | ||
{ value: 1547294400000, label: '2019-01-12', position: 145.84583333333333 }, | ||
{ value: 1547337600000, label: '2019-01-12', position: 206.19583333333333 }, | ||
{ value: 1547380800000, label: '2019-01-13', position: 266.54583333333335 }, | ||
{ value: 1547424000000, label: '2019-01-13', position: 326.8958333333333 }, | ||
{ value: 1547467200000, label: '2019-01-14', position: 387.24583333333334 }, | ||
{ value: 1547510400000, label: '2019-01-14', position: 447.59583333333336 }, | ||
{ value: 1547553600000, label: '2019-01-15', position: 507.9458333333333 }, | ||
{ value: 1547596800000, label: '2019-01-15', position: 568.2958333333333 }, | ||
]); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -87,7 +87,6 @@ export function computeAxisTicksDimensions( | |
if (axisSpec.hide) { | ||
return null; | ||
} | ||
|
||
const scale = getScaleForAxisSpec( | ||
axisSpec, | ||
xDomain, | ||
|
@@ -157,6 +156,7 @@ export function getScaleForAxisSpec( | |
range, | ||
ticks: axisSpec.ticks, | ||
integersOnly: axisSpec.integersOnly, | ||
duplicateTicks: axisSpec.duplicateTicks, | ||
}); | ||
if (yScales.has(axisSpec.groupId)) { | ||
return yScales.get(axisSpec.groupId)!; | ||
|
@@ -171,6 +171,7 @@ export function getScaleForAxisSpec( | |
enableHistogramMode, | ||
ticks: axisSpec.ticks, | ||
integersOnly: axisSpec.integersOnly, | ||
duplicateTicks: axisSpec.duplicateTicks, | ||
}); | ||
} | ||
} | ||
|
@@ -225,24 +226,23 @@ export const getMaxBboxDimensions = ( | |
}; | ||
}; | ||
|
||
function computeTickDimensions( | ||
export function computeTickDimensions( | ||
scale: Scale, | ||
tickFormat: TickFormatter, | ||
bboxCalculator: BBoxCalculator, | ||
axisConfig: AxisConfig, | ||
tickLabelPadding: number, | ||
tickLabelRotation = 0, | ||
// duplicateTicks: boolean = false, | ||
tickFormatOptions?: TickFormatterOptions, | ||
) { | ||
const tickValues = scale.ticks(); | ||
const tickLabels = tickValues.map((d) => { | ||
return tickFormat(d, tickFormatOptions); | ||
}); | ||
|
||
const { | ||
tickLabelStyle: { fontFamily, fontSize }, | ||
} = axisConfig; | ||
|
||
const { | ||
maxLabelBboxWidth, | ||
maxLabelBboxHeight, | ||
|
@@ -252,7 +252,6 @@ function computeTickDimensions( | |
getMaxBboxDimensions(bboxCalculator, fontSize, fontFamily, tickLabelRotation, tickLabelPadding), | ||
{ maxLabelBboxWidth: 0, maxLabelBboxHeight: 0, maxLabelTextWidth: 0, maxLabelTextHeight: 0 }, | ||
); | ||
|
||
return { | ||
tickValues, | ||
tickLabels, | ||
|
@@ -444,14 +443,42 @@ export function getAvailableTicks( | |
|
||
return [firstTick, lastTick]; | ||
} | ||
return ticks.map((tick) => { | ||
return getDuplicateTicks(axisSpec, scale, offset, tickFormatOptions); | ||
} | ||
|
||
export function getDuplicateTicks( | ||
axisSpec: AxisSpec, | ||
scale: Scale, | ||
offset: number, | ||
tickFormatOptions?: TickFormatterOptions, | ||
) { | ||
const ticks = scale.ticks(); | ||
const labels = [...new Set(ticks.map((tick) => axisSpec.tickFormat(tick, tickFormatOptions)))]; | ||
const allTicks = ticks.map((tick) => { | ||
return { | ||
value: tick, | ||
label: axisSpec.tickFormat(tick, tickFormatOptions), | ||
position: scale.scale(tick) + offset, | ||
}; | ||
}); | ||
|
||
if (axisSpec.duplicateTicks === false) { | ||
const uniqueTickLabels: { value: any; label: string; position: number }[] = []; | ||
allTicks.filter((value, index) => { | ||
for (let i = 0; i < labels.length; i++) { | ||
if (labels[i] === allTicks[index].label) { | ||
uniqueTickLabels.push(allTicks[index]); | ||
// once uniqueTickLabels has the unique label, remove the label from the labels array so it doesnt create a duplicate | ||
labels.splice(i, 1); | ||
} | ||
} | ||
}); | ||
return uniqueTickLabels; | ||
} else { | ||
return allTicks; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this code can be simplified a bit, let's chat on that directly There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks a ton for your help with me on this. I added commit 594c6f6 which I think gets done what we discussed, but lmk what you think! |
||
} | ||
|
||
export function getVisibleTicks(allTicks: AxisTick[], axisSpec: AxisSpec, axisDim: AxisTicksDimensions): AxisTick[] { | ||
// We sort the ticks by position so that we can incrementally compute previousOccupiedSpace | ||
allTicks.sort((a: AxisTick, b: AxisTick) => a.position - b.position); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -525,11 +525,14 @@ export interface AxisSpec extends Spec { | |
style?: AxisStyle; | ||
/** Show only integar values **/ | ||
integersOnly?: boolean; | ||
/** Remove duplicate ticks, default is false*/ | ||
duplicateTicks?: boolean; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rshen91 Maybe we should introduce a breaking change and enabling this by default changing the name to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the naming a lot better - please see commit 5037110 for naming changes |
||
} | ||
|
||
export type TickFormatterOptions = { | ||
timeZone?: string; | ||
}; | ||
|
||
export type TickFormatter = (value: any, options?: TickFormatterOptions) => string; | ||
|
||
export interface AxisStyle { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -139,6 +139,8 @@ interface ScaleOptions { | |
isSingleValueHistogram: boolean; | ||
/** Show only integer values **/ | ||
integersOnly?: boolean; | ||
/** Show duplicate tick values default to false */ | ||
duplicateTicks?: boolean; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why we need this in the scale option? the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah thanks for spotting this, I didn't remove this properly after enabling the feature - I'm removing this in commit 5037110 |
||
} | ||
const defaultScaleOptions: ScaleOptions = { | ||
bandwidth: 0, | ||
|
@@ -149,6 +151,7 @@ const defaultScaleOptions: ScaleOptions = { | |
ticks: 10, | ||
isSingleValueHistogram: false, | ||
integersOnly: false, | ||
duplicateTicks: false, | ||
}; | ||
export class ScaleContinuous implements Scale { | ||
readonly bandwidth: number; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should leverage packages to your advantage. Like using duration from moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! Pls see commit 55bf6ee for changes 💪