Refactoring: Adapt getTicks to increase reusability of code

[nikolasrieble]

Description

This PR only refactors and does not change the behaviour.

Refactoring:

• Move CartesianTickItem type into types file
• Improve getNumberIntervalTicks by using getEveryNth instead of filtering. (This function will be reused in an upcoming PR).
• Change getTicks to return the tick in both getTicksStart and getTicksEnd before filtering the result. Doing so enables me to reuse the function for an upcoming PR. The filtering is moved to a single share place.
• Adapt tests to use a mock of getStringSize that estimates the string size based on the string length. That enables me to write better tests in the upcoming PR.

The refactoring makes most sense when viewed in context of the next PR #3392.

To keep the blast radius small of the changes, I yet decided to split it into a separate PR.

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

[petii]

I'm not sure I understand the reasoning for

Adapt tests to use a mock of getStringSize that estimates the string size based on the string length. That enables me to write better tests in the upcoming PR.

This seems a bit strange from the perspective that "This PR only refactors and does not change the behaviour." but the tests change that are supposed to validate that behaviour is kept the same.

Also why do the tests need to be adapted and not new ones written in the upcoming PR?

[nikolasrieble]

This seems a bit strange from the perspective that "This PR only refactors and does not change the behaviour." but the tests change that are supposed to validate that behaviour is kept the same.

I'm not sure I understand the reasoning for

Adapt tests to use a mock of getStringSize that estimates the string size based on the string length. That enables me to write better tests in the upcoming PR.

This seems a bit strange from the perspective that "This PR only refactors and does not change the behaviour." but the tests change that are supposed to validate that behaviour is kept the same.

Also why do the tests need to be adapted and not new ones written in the upcoming PR?

Well, basically this change was required to be able to properly test equidistant ticks at all. Thus I changed it in the upcoming PR.

Yet I wanted to strip the PR with the new feature from everything must not necessarily be part of it, to make it as small as possible and reviewing as easy as possible.

If you mean to argue that I could also split this PR into two - refactoring - and changes to the test - then sure, I can.

[petii]

This could be on it's own. Maybe in something like src/util/ArrayUtils.ts?

[ckifer]

Yeah this isn't 'tick' specific per se.

Also if we're going to move to different naming conventions for utils and for our files in general lets discuss.

We'll end up having files in util/SomeUtilFile.ts as well as places like cartesian/ticks/utils. Which pattern do we follow? imo we should stick to something consistent for ease of contribution

[nikolasrieble]

I find that things should be as close to their usage as possible. If used all across the places, a top level utils folder makes sense. But if used only within CartesianAxis, I would argue that is where it should live.

[petii]

Right now it's only used in CartesianAxis. But when the need for the functionality comes along outside of tick or cartesian it's much easier to miss it if it's hidden a few subfolders deep. (Which then leads to just duplicating some similar logic ad-hoc where and when it's needed)

[nikolasrieble]

Fair enough. For consistency I will move it back to a shared utils.

[petii]

My personal preference is for the early returns as in before the changes, but since you moved the isShow filtering here it makes sense to try and reduce code duplication.

However it could also make sense that to not stop here but create and use an additional layer like

function getShownTicks(...) {
  return getTicks(...).filter(entry => entry.isShow);
}

But then you have to go and change every usage of getTicks to keep existing behaviour (and it probably does not work too well with the following PR?).

Alternatively, if we would want to go one step further in reducing code duplication we could introduce a holder for these like

const tickGeneratorForInterval: Record<
  CartesianAxisProps['interval'],
  (args: Omit<CartesianAxisProps, 'tickMargin'>) => CartesianTickItem[]
> = { 
  'preserveEnd': getTicksEnd, 
  'preserveStart': getTicksStart, 
  'preserveStartEnd': (args) => getTicksStart(args, true),
}

and then just

  const candidates = tickGeneratorForInterval[interval]({ ... });
  return candidates.filter(entry => entry.isShow);

to get rid of hide the branching altogether

[nikolasrieble]

I like the ideas, and will most probably come back to them - but only when following up with this file after landing the feature enhancement. For now I want to only refactor as much as I need to, in order to nicely implement the equidistant ticks.

[petii]

Looking at the following PR this also has the benefit that adding the new option is just

  'equidistantPreserveStart': (args) => getEveryNthTick(getTicksStart(args)),

instead of essentially adding back the duplicated code that this PR removed. With that context I would argue even more so that the conditions should still be separate and not share the branch for 'preserveStart' and 'preserveStartEnd'.

[petii]

If you mean to argue that I could also split this PR into two - refactoring - and changes to the test - then sure, I can.

I'm not entirely sure about refactoring best practices whether it makes sense to refactor and the change the tests or change the tests and then refactor, however my question is more directed towards setting precedence I guess.

[ckifer]

few nits, but overall looks good.

Curious about util placement moving forwards

[ckifer]

Yeah this isn't 'tick' specific per se.

Also if we're going to move to different naming conventions for utils and for our files in general lets discuss.

We'll end up having files in util/SomeUtilFile.ts as well as places like cartesian/ticks/utils. Which pattern do we follow? imo we should stick to something consistent for ease of contribution

[ckifer]

I don't think this adds any indication of actual text length. Better maybe than no variation, but certainly not accurate.

[nikolasrieble]

Agree, but this enables me to test with larger labels on different positions (i.e. first tick, second tick, third tick, etc.)