-
Notifications
You must be signed in to change notification settings - Fork 16
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
AG-10444 Increase axis ticks on zoom #3329
Conversation
Snapshots automatically updated, please review before merge: diff |
Snapshots automatically updated, please review before merge: diff |
1 similar comment
Snapshots automatically updated, please review before merge: diff |
Snapshots automatically updated, please review before merge: diff |
Update E2E snapshots.
...ts-website/e2e/zoom.spec.ts-snapshots/zoom-2-before-navigator-drag-y-axis-chromium-linux.png
Outdated
Show resolved
Hide resolved
.../ag-charts-website/e2e/zoom.spec.ts-snapshots/zoom-minuskey-focus-visible-chromium-linux.png
Outdated
Show resolved
Hide resolved
Snapshots automatically updated, please review before merge: diff |
tickLayoutDomain = this.calculateTickLayout(domain, NiceMode.TickAndDomain, [0, 1]).niceDomain; | ||
} | ||
|
||
let niceMode: NiceMode; |
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 fixes an issue where ticks would be in the wrong place. You do want to use a nice domain, but the nice domain of the zoomed out series. This means when generating the ticks, you'll still have nice ticks, but the domain won't be adjusted
@@ -9,7 +9,7 @@ export interface ScaleTickParams<I> { | |||
} | |||
|
|||
export interface ScaleFormatParams<D> { | |||
visibleTicks: D[]; | |||
domain: D[]; | |||
ticks: D[]; |
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.
Ticks is now the visible ticks
override ticks( | ||
params: ScaleTickParams<TimeInterval | number>, | ||
domain: Date[] = this.domain, | ||
visibleRange: [number, number] = [0, 1] |
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.
All scales now use visible range, and will only return the visible ticks
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.
LGTM, maybe @iMoses should review too?
|
||
let tickDomain: D[] = niceDomain; | ||
let rawTicks: any[]; | ||
|
||
// @todo(xxx) - removing this references makes TS errors | ||
const scaleStopTsComplaining = scale; |
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.
Nit: Is this still needed, I tried removing it locally and don't see any TS errors?
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.
LGTM
}) { | ||
const t = getTickInterval(start, stop, tickCount, minTickCount, maxTickCount); | ||
return t ? t.range(new Date(start), new Date(stop)) : []; // inclusive stop | ||
return t ? t.range(new Date(start), new Date(stop), undefined, visibleRange) : []; // inclusive stop |
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.
nit: be explicit with the extend
parameter and supply false
instead of undefined
ditto for the other 2 cases on this file
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.
Passing in an options object instead
@@ -474,38 +474,56 @@ export abstract class Axis< | |||
this.animatable = animatable; | |||
} | |||
|
|||
_niceDomainRange: number = NaN; | |||
_scaleNiceDomainRange: number = NaN; |
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.
nit: maybe call it _scaleNiceDomainRangeExtent
, not to be confused with a range, which is what we conventionally call an array of min, max?
this.dataDomain.domain, | ||
visibleRange, | ||
initialPrimaryTickCount | ||
); | ||
|
||
const range = findRangeExtent(this.range); |
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.
nit: rename to rangeExtent
?
https://ag-grid.atlassian.net/browse/AG-10444