feat: allow ticks to be configurable with total count, interval, and starting placement #330
Conversation
TCL735
requested review from
a team,
kristinarobinson,
hoorayimhelping and
Palakp41
and removed request for
a team
|
|
||
| - **yTickStep**: _number. Optional. **Ignored when yTicks is specified**._ A number representing the tick interval for the y-axis. May be negative. | ||
|
|
||
| - **yTickStart**: _number. Optional. **Ignored when yTicks is specified**._ A number representing the value of the first tick on the y-axis. This will determine the placement of all subsequent ticks. Must be within the domain of the y-axis, otherwise will not be rendered. Subsequent ticks may still be rendered if their placement is within the domain. | ||
|
|
There was a problem hiding this comment.
initial reaction is that this is a bit alarming and the interface is pretty hefty. the interplay between all these different properties (e.g. if xTickStart is outside the domain of the x-axis, they aren't rendered) is a little complex and confusing. not sure if there's anything to do about this now, but i think we'll need to address the issues that are causing big configurations like this
There was a problem hiding this comment.
Slight clarification and change to the text of xTickStart and yTickStart.
What do you think of this?
giraffe/src/utils/getTextMetrics.ts
Outdated
| // when it includes a dash (negative number), it tends to be skinnier than | ||
| // a positive number because a dash is not as wide as a single digit (most of the time). | ||
| // Add padding when text has a dash by making dashes twice as wide. | ||
| const widthPadding = |
There was a problem hiding this comment.
is there a cleaner way of doing this? i don't think we can rely on a dash being the same thing as a negative indicator. some platforms might not render -1 consistently with other platforms
There was a problem hiding this comment.
Yes there is a cleaner way. No extra <span> necessary. We do need to check for a dash though, unfortunately. I will send up a new commit in the near future.
| expect(result.length).toEqual(0) | ||
| }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
great tests from a tactical level. from a higher, strategic level, just reinforcing what i said before - there's a lot of interplay here that is pretty complex and will probably be really confusing for users and might be an indication of larger design issues.
| tickStep?: number | ||
| ) => { | ||
| const [start = 0, end = 0] = domain | ||
| if (isFiniteNumber(totalTicks) || isFiniteNumber(tickStep)) { |
There was a problem hiding this comment.
should this || be an &&?
There was a problem hiding this comment.
No. It is correct as ||. I will demo this to show off the flexibility.
| expect(isFiniteNumber(() => 123)).toEqual(false) | ||
| expect(isFiniteNumber(Symbol())).toEqual(false) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
for consistency, can you change these tests to its please?
Closes #79
Allows user to specify tick generation in the config.