-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
CartesianAxis: Improve interval option 'equidistantPreserveStart' #3768
Merged
Merged
Changes from all commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
abe0583
First draft
695514f
Functional implementation: Equidistant ticks
f2b74bc
Extend Axisinterval type and add explanation
f79f31c
Remove console logs
65cbf21
Extract TickUtils.spec.ts
6ba9807
Make eslint happy
998d87d
TickUtils: Eslint must be happy
26fb1ec
getTicks: Eslint must be happy
c7f51d2
TickPositioning: Cleanup story
ad76465
Merge remote-tracking branch 'origin' into nikolas.ticks.equidistant
5ae63d5
Merge branch 'master' into nikolas.ticks.equidistant
6852d43
Cleanup merge conflict properly
cb8ed8d
Apply suggestions from code review
0f0fd91
Merge branch 'nikolas.ticks.equidistant' of https://github.com/rechar…
ee4eb1e
Storybook: Add margin
257aef7
Add unit tests, and fix one edge case
9d49913
Review: Do not use lodash
bf89acd
Review: Improve equidistantPreserveStart
86fa0e3
Fix test
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
import { isVisible } from '../util/TickUtils'; | ||
import { CartesianTickItem } from '../util/types'; | ||
import { getEveryNthWithCondition } from '../util/getEveryNthWithCondition'; | ||
import { Sign } from './getTicks'; | ||
|
||
export function getEquidistantTicks( | ||
sign: Sign, | ||
boundaries: { start: number; end: number }, | ||
getTickSize: (tick: CartesianTickItem, index: number) => number, | ||
ticks: CartesianTickItem[], | ||
minTickGap: number, | ||
): CartesianTickItem[] { | ||
const result = (ticks || []).slice(); | ||
|
||
const { start: initialStart, end } = boundaries; | ||
let index = 0; | ||
// Premature optimisation idea 1: Estimate a lower bound, and start from there. | ||
// For now, start from every tick | ||
let stepsize = 1; | ||
let start = initialStart; | ||
|
||
while (stepsize <= result.length) { | ||
// Given stepsize, evaluate whether every stepsize-th tick can be shown. | ||
// If it can not, then increase the stepsize by 1, and try again. | ||
|
||
const entry = ticks?.[index]; | ||
|
||
// Break condition - If we have evaluate all the ticks, then we are done. | ||
if (entry === undefined) { | ||
return getEveryNthWithCondition(ticks, stepsize); | ||
} | ||
|
||
// Check if the element collides with the next element | ||
const size = getTickSize(entry, index); | ||
|
||
const tickCoord = entry.coordinate; | ||
// We will always show the first tick. | ||
const isShow = index === 0 || isVisible(sign, tickCoord, size, start, end); | ||
|
||
if (!isShow) { | ||
// Start all over with a larger stepsize | ||
index = 0; | ||
start = initialStart; | ||
stepsize += 1; | ||
} | ||
|
||
if (isShow) { | ||
// If it can be shown, update the start | ||
start = tickCoord + sign * (size / 2 + minTickGap); | ||
index += stepsize; | ||
} | ||
} | ||
|
||
return []; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
// unit test for getEquidisantTicks | ||
|
||
import { getEquidistantTicks } from '../../src/cartesian/getEquidistantTicks'; | ||
import { CartesianTickItem } from '../../src/util/types'; | ||
|
||
describe('getEquidistantTicks', () => { | ||
// We mock getting the tick width by simply using the value of the tick. | ||
// This makes testing rather easy and intuitive. | ||
const getTickSize = (tick: CartesianTickItem) => { | ||
return tick.value; | ||
}; | ||
|
||
it('should return empty array if no ticks are passed', () => { | ||
const result = getEquidistantTicks(1, { start: 0, end: 100 }, getTickSize, [], 0); | ||
expect(result).toEqual([]); | ||
}); | ||
|
||
test.each([ | ||
// If all ticks fit, we show all ticks | ||
{ ticksThatFit: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9], resultingTicks: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9] }, | ||
{ ticksThatFit: [0, 1, 2, 3, 4, 5, 6, 7, 8], resultingTicks: [0, 2, 4, 6, 8] }, | ||
{ ticksThatFit: [0, 1, 2, 3, 4, 5, 6, 7], resultingTicks: [0, 5] }, | ||
{ ticksThatFit: [0, 1, 2, 3, 4, 5, 6], resultingTicks: [0, 5] }, | ||
{ ticksThatFit: [0, 1, 2, 3, 4, 5], resultingTicks: [0, 5] }, | ||
{ ticksThatFit: [0, 1, 2, 3, 4], resultingTicks: [0] }, | ||
{ ticksThatFit: [0, 1, 2, 3], resultingTicks: [0] }, | ||
{ ticksThatFit: [0, 1, 2], resultingTicks: [0] }, | ||
{ ticksThatFit: [0, 1], resultingTicks: [0] }, | ||
{ ticksThatFit: [0], resultingTicks: [0] }, | ||
// We always show the first tick, even if it does not fit | ||
{ ticksThatFit: [1, 2, 3, 4, 5, 6, 7, 8, 9], resultingTicks: [0] }, | ||
])('Show only every n-th tick that fits, but always show the first.', ({ ticksThatFit, resultingTicks }) => { | ||
const ticks = [] as CartesianTickItem[]; | ||
for (let index = 0; index < 10; index++) { | ||
ticks.push({ value: ticksThatFit.includes(index) ? 10 : 1000, coordinate: index * 50 }); | ||
} | ||
const result = getEquidistantTicks(1, { start: 0, end: 10 * 50 }, getTickSize, ticks, 0); | ||
expect(result).toEqual(resultingTicks.map(index => ticks[index])); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -234,7 +234,6 @@ describe('getTicks', () => { | |
[ | ||
{ | ||
coordinate: 50, | ||
tickCoord: 50, | ||
value: '10', | ||
}, | ||
], | ||
|
This file was deleted.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This section deserves scrutiny. Is this the behaviour we want to achieve?
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.
is this really true in the browser though?
I feel like all of these should be able to fit? every time?
I kind of think we shouldn't always preserve start and instead show as many as we can while maintaining equidistance
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 construct the ticks in the test such that this is true in the browser.
But good point, if we were to dynamically allow starting from any tick but still only show equidistant ticks, we could show much more.
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.
Really, we could consider to redo the interface completely. As far as I understand it, the options currently are:
We could have the option to choose
equidistant
withoutalwaysShowStart
.I admittedly find the naming of these strategies not that insightful. We should probably investigate how others do this.
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.
The more I think about this, I believe we should simply replace the existing
equidistantPreserveStart
with this implementation.This would be a breaking change, as it changes existing behaviour, but the change would only be to the better, namely we would show more ticks than we previously did, and there would yet be no collision.
@ckifer What do you think about this approach?
I like it, because it does not further complicate the API and it improves the existing behaviour (almost a bug fix)
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'm on board - if people complain that their chart looks better then... not sure lol. If anything we could do the same thing with preserveEnd if we wanted no?
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.
Done. Thanks for the discussion.
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.
They will look better, Coltin! :P
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.
Exactly 🚀. If there are complaints I will say but... they look better 😂