-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[ML] Explorer Chart Tweaks #22955
[ML] Explorer Chart Tweaks #22955
Changes from 10 commits
1ccd426
953eff5
8b0b67a
678bdb9
a8a2c9d
11fe572
aa48d6b
64c4ded
592a975
51599f6
64114b6
49c1d1b
bf3e296
070d1c6
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 |
---|---|---|
|
@@ -187,3 +187,132 @@ export function numTicksForDateFormat(axisWidth, dateFormat) { | |
const tickWidth = calculateTextWidth(moment().format(dateFormat), false); | ||
return axisWidth / (1.75 * tickWidth); | ||
} | ||
|
||
// Based on a fixed starting timestamp and an interval, get tick values within | ||
// the bounds of earliest and latest. This is useful for the Anomaly Explorer Charts | ||
// to align axis ticks with the gray area resembling the swimlane cell selection. | ||
export function getTickValues(startTs, tickInterval, earliest, latest) { | ||
const tickValues = [startTs]; | ||
|
||
function addTicks(ts, operator) { | ||
let newTick; | ||
let addAnotherTick; | ||
|
||
switch (operator) { | ||
case 'previous': | ||
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. the |
||
newTick = ts - tickInterval; | ||
addAnotherTick = newTick >= earliest; | ||
break; | ||
case 'next': | ||
newTick = ts + tickInterval; | ||
addAnotherTick = newTick <= latest; | ||
break; | ||
} | ||
|
||
if (addAnotherTick) { | ||
tickValues.push(newTick); | ||
addTicks(newTick, operator); | ||
} | ||
} | ||
|
||
addTicks(startTs, 'previous'); | ||
addTicks(startTs, 'next'); | ||
|
||
tickValues.sort(); | ||
|
||
return tickValues; | ||
} | ||
|
||
const TICK_DIRECTION = { | ||
NEXT: 'next', | ||
PREVIOUS: 'previous' | ||
}; | ||
|
||
// This removes overlapping x-axis labels by starting off from a specific label | ||
// that is required/wanted to show up. The code then traverses to both sides along the axis | ||
// and decides which labels to keep or remove. All vertical tick lines will be kept visible, | ||
// but those which still have their text label will be emphasized using the ml-tick-emphasis class. | ||
export function removeLabelOverlap(axis, startTs, tickInterval, width) { | ||
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 'startTimestamp' would be a clearer name for this argument, assuming 'Ts' is a timestamp? |
||
// Put emphasis on all tick lines, will again de-emphasize the | ||
// ones where we remove the label in the next steps. | ||
axis.selectAll('g.tick').select('line').classed('ml-tick-emphasis', true); | ||
|
||
function getNeighourTickFactory(operator) { | ||
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. Typo in the function name. Should be |
||
return function (ts) { | ||
switch (operator) { | ||
case TICK_DIRECTION.PREVIOUS: | ||
return ts - tickInterval; | ||
case TICK_DIRECTION.NEXT: | ||
return ts + tickInterval; | ||
} | ||
}; | ||
} | ||
|
||
function getTickDataFactory(operator) { | ||
const getNeighourTick = getNeighourTickFactory(operator); | ||
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. Same typos here. Should be 'Neighbor' / 'Neighbour'. |
||
const fn = function (ts) { | ||
const filteredTicks = axis.selectAll('.tick').filter(d => d === ts); | ||
|
||
if (filteredTicks[0].length === 0) { | ||
return false; | ||
} | ||
|
||
const tick = d3.selectAll(filteredTicks[0]); | ||
const textNode = tick.select('text').node(); | ||
|
||
if (textNode === null) { | ||
return fn(getNeighourTick(ts)); | ||
} | ||
|
||
const tickWidth = textNode.getBBox().width; | ||
const padding = 15; | ||
// To get xTransform it would be nicer to use d3.transform, but that doesn't play well with JSDOM. | ||
// So this uses a regex variant because we definitely want test coverage for the label removal. | ||
// Once JSDOM supports SVGAnimatedTransformList we can use the simpler version. | ||
// const xTransform = d3.transform(tick.attr('transform')).translate[0]; | ||
const xTransform = +(/translate\(\s*([^\s,)]+)[ ,]([^\s,)]+)/.exec(tick.attr('transform'))[1]); | ||
const xMinOffset = xTransform - (tickWidth / 2 + padding); | ||
const xMaxOffset = xTransform + (tickWidth / 2 + padding); | ||
|
||
return { | ||
tick, | ||
ts, | ||
xMinOffset, | ||
xMaxOffset | ||
}; | ||
}; | ||
return fn; | ||
} | ||
|
||
function checkTicks(ts, operator) { | ||
const getTickData = getTickDataFactory(operator); | ||
const currentTickData = getTickData(ts); | ||
|
||
if (currentTickData === false) { | ||
return; | ||
} | ||
|
||
const getNeighourTick = getNeighourTickFactory(operator); | ||
const newTickData = getTickData(getNeighourTick(ts)); | ||
|
||
if ( | ||
newTickData !== false | ||
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. this style with just one condition looks a bit odd. |
||
) { | ||
if ( | ||
newTickData.xMinOffset < 0 || | ||
newTickData.xMaxOffset > width || | ||
(newTickData.xMaxOffset > currentTickData.xMinOffset && operator === TICK_DIRECTION.PREVIOUS) || | ||
(newTickData.xMinOffset < currentTickData.xMaxOffset && operator === TICK_DIRECTION.NEXT) | ||
) { | ||
newTickData.tick.select('text').remove(); | ||
newTickData.tick.select('line').classed('ml-tick-emphasis', false); | ||
checkTicks(currentTickData.ts, operator); | ||
} else { | ||
checkTicks(newTickData.ts, operator); | ||
} | ||
} | ||
} | ||
|
||
checkTicks(startTs, TICK_DIRECTION.PREVIOUS); | ||
checkTicks(startTs, TICK_DIRECTION.NEXT); | ||
} |
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.
I don't really understand what this is saying. Maybe something like 'and the highlighted area would span the entire chart'?