Improved category axes conversions #1748
Conversation
... in Plots.doCalcdata - this approach was flawed: led to position value not updating properly on calc-less update (e.g. zoom/pan)
- DO NOT convert category position value to indices (the old 'r') in Axes.coercePosition - as this relied on ax._categories which isn't defined during Plots.supplyDefaults. - modify `type: 'category'` conversions functions accordingly (a bit hacky) - update annotation/click.js to reflect change in 'r' definition - use ax.cleanPos to clean position value per axis type in coercePosition
- as now Axes.coercePosition does not rely on ax._categories for `type: 'category' axes.
- a little wrapper around ax.cleanPos - use it for annotations *xclick* and *yclick*
src/lib/index.js
Outdated
| @@ -87,6 +92,13 @@ lib.pushUnique = require('./push_unique'); | |||
|
|
|||
| lib.cleanNumber = require('./clean_number'); | |||
|
|
|||
| lib.num = function num(v) { | |||
There was a problem hiding this comment.
Do we want a more descriptive name for this now that it's not just internal to one module? ensureNumber? safeNumber?
There was a problem hiding this comment.
I went with ensureNumber in 071249f
src/plots/cartesian/axes.js
Outdated
|
|
||
| if(axRef === 'paper' || axRef === 'pixel') { | ||
| ax = { cleanPos: Lib.num }; |
There was a problem hiding this comment.
don't need to pretend we're making an axis anymore... just cleanPos = Lib.num or cleanPos = ax.cleanPos
| if(hasColumns(trace)) { | ||
| convertColumnData(trace, xa, ya, 'x', 'y', ['z']); | ||
| x = trace.x; | ||
| y = trace.y; |
There was a problem hiding this comment.
Oh interesting... were we double-converting triplet data on category and date axes before? Do we have a test of these cases?
There was a problem hiding this comment.
were we double-converting triplet
Right. Previously ax.d2c was called twice in heatmap/calc.js. For example, on master the heatmap_xyz-dates-and-categories mock has:
gd._fullLayout.yaxis._categories
// ["3-month", "6-month", "1-year", "2-year", "3-year", "5-year", 0, 1, 2, 3, 4, 5]which still gives the correct result though as the string categories are ignored during r2p.
Now that, starting in this PR, r2p convert strings to pixels, this patch is now necessary to make the heatmap_xyz-dates-and-categories pass.
Do we have a test of these cases?
Good call. I added heatmap/calc test case to 🔒 this down more explicitly in 4434fad
| expect(layoutOut.annotations[1]._xclick).toBe(1, 'linear'); | ||
| expect(layoutOut.annotations[1]._yclick).toBe(undefined, 'invalid date'); | ||
| expect(layoutOut.annotations[2]._xclick).toBe(2, 'log'); | ||
| expect(layoutOut.annotations[2]._yclick).toBe('A', 'category'); |
| it('should handle the category x/y/z/ column case', function() { | ||
| var out = _calc({ | ||
| x: ['a', 'a', 'a', 'b', 'b', 'b', 'c', 'c', 'c'], | ||
| y: ['A', 'B', 'C', 'A', 'B', 'C', 'A', 'B', 'C'], |
There was a problem hiding this comment.
can we test dates too? That's the other case where d2c shouldn't be called multiple times - actually, that results in a timezone shift because of our legacy support for local milliseconds!
ax.d2c('1970-01-02')
86400000
ax.d2c(ax.d2c('1970-01-02'))
68400000
ax.d2c(ax.d2c(ax.d2c('1970-01-02')))
50400000
|
Looks great! Thanks for the tweaks. |
- same as what Axes.coercePosition does at the defaults step for other axis types - post #1748, we need to handle this at the ax.r2c as the categories are only known after the calc step.
fixes #1720
This PR is a rebased version of the
component-category-axes-relayout-attemptbranch with better commit messages.In brief, category (i.e. string) values of annotation/shape/image position attributes (e.g.
x,y,x0...) referencingtype: 'category'axes are no longer converted to indices (i.e. numbers) during the defaults step. This implies:ax._categories(which isn't defined duringPlots.supplyDefaultswhich led to 🐛 shapes on category axis are redrawn incorrectly after zoom #1720 ) and this ugly category-axes-only block.rAxes.setConvertdata type means for category axes - see commit b67657f for more details. This part isn't perfect, but better than what we have currently accordingly to @alexcjohnson