Refactor Axes.doTicks and Axes.doTicksSingle #3254

etpinard · 2018-11-16T22:14:39Z

please review this PR commit-by-commit

Axes.doTicksSingle is currently used to compute and draw ticks, but also grid lines, tick labels, axis titles ... and a few more things. It is used by cartesian axes (via Axes.doTicks), colorbars, ternary axes and polar axes. Making that one single function work for all these cases turned Axes.doTicksSingle into hackcity 😨

In brief, this PR:

first breaks up Axes.doTicksSingle into drawTicks, drawGrid, drawZeroLine and drawLabels
wraps the cartesian ticks, grid, label, title sequence into a method called Axes.drawOne
renames Axes.doTicks -> Axes.draw, this one draws multiple (or all) cartesian axes
breaks up a few more reusable parts from Axes.drawOne
breaks up drawTitle, calcBoundingBox and doAutoMargin- which only get called by cartesian axes - from drawLabels
adapts colorbar, ternary and polar axis drawing code
removes a few hacks in ternary and polar subplots. I'm particularly 😄 about 944482c

Note: commit 5858067 is potentially dangenerous. No tests are failing because of it, but ignoring this comment:

plotly.js/src/plots/cartesian/axes.js

Lines 1940 to 1943 in 3a32ac0

    
           // update the axis title 
        
           // (so it can move out of the way if needed) 
        
           // TODO: separate out scoot so we don't need to do 
        
           // a full redraw of the title (mostly relevant for MathJax)

makes me nervous.

A few things in axes.js could still be ironed out to make it even more "reusable":

a lot of things depend on ax._id.chaAt(0), this could made more general by adding a new opts flag e.g. orientation: 'h' | 'v'
tick sign logic is still a bit of a mess
clip-or-not-to-clip axis ticks logic is not great either
Axes.drawLabels could be made more d3-esque, but I worrying about potential performance losses.

but hopefully you'll find this PR a step in the right direction.

cc @plotly/plotly_js

TODO:

finish ✏️ jsDocs after initial 👍 -- done in: ea6e76f

- the companion Axes methods are: drawTicks, drawGrid, drawZeroLine, drawLabels, drawTitle - axis-dependent params and functions are set as opts in drawOne - stash tickLabels selection on ax._tickLabels to use it in drawTitle w/o having to query the DOM again - some linting here and there.

... that uses Axes.drawOne, instead of Axes.doTicksSingle

... just like in the ternary case, but could potentially be used in elsewhere.

- say bye-bye to Axes.doTicks - remove a few not obsolete '-' field on ternary subplot axis objects.

- call full updateRadialAxis on radial drag and call full updateAngularAxis on angular drag. - no need to nest grid lines into extra <g> (that was an artifact from doTicksSingle. - ... more cleanup to come.

... to turn off crispEdge SVG rendering.

alexcjohnson · 2018-11-17T05:49:09Z

@etpinard this is awesome. Huge step in the right direction!

Is all of this stuff sufficiently generalized that we could call most of it components/axes/ and leave in cartesian/axes.js only the stuff that's really specific to cartesian? Wouldn't have to do it now, but... is there going to be a better time?

commit 5858067 is potentially generous. No tests are failing because of it, but ignoring this comment...

Other than reading that comment I have no recollection of what's going on there... so I guess we just test what it seems to be worried about, which is dynamic title scoot. Do we have such tests? In the mathjax mock I see that both tick labels and axis title support mathjax, I guess in principle we should have 4 test cases: (regular/mathjax tick labels) * (regular/mathjax title), shifting back and forth between collision and no collision and verifying that the title moves to the right place.

a lot of things depend on ax._id.chaAt(0), this could made more general by adding a new opts flag e.g. orientation: 'h' | 'v'

Or even ax._orientation - you're moving away from that pattern in order to make the axis objects passed to the drawing routines require less mocking, but I feel like this one is meaningful enough as part of the axis' identity - does it ever want different values for the same axis in different contexts? Anyway it's used in some other places besides the drawing code - setConvert, scattered about the autotick & tick value/text code, and other places.

Anyway aside from ensuring we have tests for 5858067 I wouldn't consider any of my comments ^^ blocking, just opportunities to push this even farther, if you've got the appetite for it.

alexcjohnson · 2018-11-17T05:59:03Z

src/plots/cartesian/axes.js

@@ -1805,7 +1805,7 @@ axes.drawTicks = function(gd, ax, opts) {
    ticks.enter().append('path')
        .classed(cls, 1)
        .classed('ticks', 1)
-        .classed('crisp', 1)
+        .classed('crisp', !opts.noCrisp)


Yeah, that's cool! But I'm not a huge fan of negated option names - could we use:
opts.crisp !== false?

Done in 016b8e4

... so that one can call it after dynamically adding MathJax <script> on page.

- so that one can include MathJax by adding a <script> for a given test.

- Removing MathJax from the page currently leads to errors in downstream tests.

etpinard · 2018-11-19T23:55:27Z

I guess in principle we should have 4 test cases: (regular/mathjax tick labels) * (regular/mathjax title), shifting back and forth between collision and no collision and verifying that the title moves to the right place.

Looks like everything is ok: 6ccc457

... by the way ⤴️ are our first MathJax jasmine tests. About time.

alexcjohnson · 2018-11-20T04:06:56Z

Love it. 💃

etpinard · 2018-11-20T16:33:46Z

Is all of this stuff sufficiently generalized that we could call most of it components/axes/ and leave in cartesian/axes.js only the stuff that's really specific to cartesian?

I gave this a go this morning. I couldn't easily decide where to put a few things (e.g. cartesian/axis_ids.js and few methods in cartesian/axes.js), so I'll defer that for later.

Thinking about this, it might be nice to make a distinction between "registerable" (i.e optional) component modules (e.g. annotations/, shapes/) and component modules that are mostly intended to DRY things up (e.g. color/, colorscale/ ... and the soon-to-be axes/). Maybe the latter could be placed in different directory e.g. src/common/ and leave src/components/ for "registerable" modules?

etpinard added 16 commits November 16, 2018 16:38

replace Axes.doTicks with Axes.draw

a2271eb

... that uses Axes.drawOne, instead of Axes.doTicksSingle

sub doTicksSingle -> drawOne in dragbox and axis transition

8f77f2c

move translate, tick, label logic into own generators

7a6d24d

use Axes.drawTicks and Axes.drawLabels in Colorbar.draw

2d8a963

generalize Axes.makeLabelFns to support pads about ticks at angles

36c1d64

... just like in the ternary case, but could potentially be used in elsewhere.

use Axes drawTicsk, drawGrid and drawLabels in ternary drawAxes

73a38fd

- say bye-bye to Axes.doTicks - remove a few not obsolete '-' field on ternary subplot axis objects.

use drawTIcks, drawGrid and drawLabels to draw polar axes

38c6ad8

- call full updateRadialAxis on radial drag and call full updateAngularAxis on angular drag. - no need to nest grid lines into extra <g> (that was an artifact from doTicksSingle. - ... more cleanup to come.

use different transFn for polar ticks, grid and labels when convenient

2247377

bye-bye Axes.doTicksSingle

d60e84d

lint (match variable name across axes.js methods)

219eb08

add 'noCrisp' option to Axes.drawTicks and Axes.drawGrid

944482c

... to turn off crispEdge SVG rendering.

add Axes.getTickSigns

7033585

move Axes.drawTitle out of Axes.drawLabels

5858067

mv axis calcBoundingBox and autoMargin from drawLabels -> drawOne

eb8e0d0

more d3-esque pattern in Axes.drawLabels

3b8f7af

etpinard added type: maintenance labels Nov 16, 2018

alexcjohnson reviewed Nov 17, 2018

View reviewed changes

etpinard added 6 commits November 19, 2018 14:18

rename noCrisp -> crisp

016b8e4

add jsDoc to new Axes methods

ea6e76f

wrap mathjax_config into function

187ffd0

... so that one can call it after dynamically adding MathJax <script> on page.

serve MathJax assets in karma tests (w/o bundling them)

9c74e94

- so that one can include MathJax by adding a <script> for a given test.

test axis title scoot, including MathJax ;)

ed7be86

mv mathjax axis title test to a bundle_test suite

6ccc457

- Removing MathJax from the page currently leads to errors in downstream tests.

etpinard merged commit 695f311 into master Nov 20, 2018

etpinard deleted the replace-axes-doTicks branch November 20, 2018 16:29

alexcjohnson mentioned this pull request Nov 20, 2018

Reorganize components/ and lib/ #3271

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Axes.doTicks and Axes.doTicksSingle #3254

Refactor Axes.doTicks and Axes.doTicksSingle #3254

etpinard commented Nov 16, 2018 •

edited

Loading

alexcjohnson commented Nov 17, 2018

alexcjohnson Nov 17, 2018

etpinard Nov 19, 2018

etpinard commented Nov 19, 2018

alexcjohnson commented Nov 20, 2018

etpinard commented Nov 20, 2018

	// update the axis title
	// (so it can move out of the way if needed)
	// TODO: separate out scoot so we don't need to do
	// a full redraw of the title (mostly relevant for MathJax)

Refactor Axes.doTicks and Axes.doTicksSingle #3254

Refactor Axes.doTicks and Axes.doTicksSingle #3254

Conversation

etpinard commented Nov 16, 2018 • edited Loading

alexcjohnson commented Nov 17, 2018

alexcjohnson Nov 17, 2018

Choose a reason for hiding this comment

etpinard Nov 19, 2018

Choose a reason for hiding this comment

etpinard commented Nov 19, 2018

alexcjohnson commented Nov 20, 2018

etpinard commented Nov 20, 2018

etpinard commented Nov 16, 2018 •

edited

Loading