Calculate hidden minor ticks if ticklabelindex is set.

[my-tien]

Closes #7423

This PR ensures that the ticklabelindex axis property has an effect even if minor ticks are not visible. Labels are then drawn at possibly invisible minor tick labels n positions away from a major tick.
This makes it possible to draw the label for a period to the left of a major tick which was previously not always possible. See this issue for two examples where it was previously not possible: #7423

Disclaimer

I am required to add that…

the software is provided "as is", without warranty of any kind, express or implied, including but not limited to the warranties of merchantability, fitness for a particular purpose and noninfringement. in no event shall the authors or copyright holders be liable for any claim, damages or other liability, whether in an action of contract, tort or otherwise, arising from, out of or in connection with the software or the use or other dealings in the software.

[emilykl]

Thanks for the PR, @my-tien! Sorry for the delay, I'll review ASAP.

Have you looked into the failing image tests yet?

[my-tien]

Hi @emilykl, not yet! I'll try to do it next week.

[my-tien]

Failing baseline image tests are resolved. I also updated date_axe_period_ticklabelindex.png and date_axe_period2_ticklabelindex.png.
In each image, the left-most label of one of the axes is now drawn when it was previously omitted. I think this is actually more correct than before.

[emilykl]

@my-tien Great! I'm taking a look.

[emilykl]

This is just a style change, correct? The code is logically equivalent before and after the change, right?

IMO it would be clearer to leave as-is, but add extra parentheses around the && clause to clarify it's evaluated first.

i.e. if(a || (b && c))

Or, otherwise, maybe

Suggested change

	if (ticklabelIndex) {
	if (allTicklabelVals.indexOf(tickVals[i]) === -1) {
	hideLabel(t);
	}
	} else {
	if (tickVals[i].skipLabel) {
	hideLabel(t);
	}
	}
	if (ticklabelIndex && allTicklabelVals.indexOf(tickVals[i]) === -1) {
	hideLabel(t);
	} else if (tickVals[i].skipLabel) {
	hideLabel(t);
	}

[my-tien]

I reverted the code. No strong opinion on my end 👍

[my-tien]

Actually, this does something different. Only if ticklabelindex is false should the tickVals[i].skipLabel check be done.

[emilykl]

I see... the outcome is different in the case where tickVals[i].skipLabel is false (or undefined), tickLabelIndex is true, and allTicklabelVals.indexOf(tickVals[i]) === -1 is also true.

Previously that would trigger a call to hideLabel(t), but with the new logic it doesn't.

[my-tien]

Hi @emilykl, since the logic here is very subtle, I went back and made the behavior clearer. Basically, my first attempt said: if ticklabelindex is present, the skipLabel flags are not valid, instead the allTicklabelVals array contains all labels that should be drawn. Now, I simply set skipLabel to true if they are not in that array and the check at this code line can just check skipLabel again.

[emilykl]

@my-tien Thank you for the PR. This change looks good to me.

I have a few requests concerning the image tests:

• Could you combine the 3 new image mocks into one single mock with 3 subplots? This will help keep the total number of image tests down and keep the CI faster.

• Could you explicitly set dtick and tick0 for the minor ticks in each mock, to make the expected behavior clear?

• We are no longer following the convention of prefixing new mocks and baselines with zz-, so you can remove that prefix from the filenames. If this causes any small changes in other baselines, you can simply commit those changed baselines as part of this PR.

I added a note to the PR description that this will close #7423; please let me know if you think that's not the case.

[emilykl]

@my-tien This looks good, thank you for the updates.

Once you push the updated baseline images I think this is ready to go.