mark-level facets #1085
mark-level facets #1085
Conversation
this changes one unit test as it does not maintain the data order in the exclude facet. but that order was kinda arbitrary anyway. Use an explicit sort transform if necessary.
add a test plot with mark-level facets and exclude clarify comments simplify proper facet look-up for grid lines
mbostock
force-pushed
the
fil/mark-facets
branch
from
eecae55 to
aaa44e6
Compare
|
I’m trying to work through two issues here:
|
There was a problem hiding this comment.
I think this PR is largely ready to go. Exceptions:
-
It’d be better to only initialize the fx and fy scales once. We currently need to initialize them once to compute their domains, and then again with the other scales in case a mark channel wants to sort the domain of fx or fy. I’m not sure it’s worth the trouble to only initialize them once, though, so this isn’t urgent.
-
I don’t think we’re respecting the scale.transform option correctly for the first initialization of fx and fy scales (see the TODO in code). We should test that because I suspect it is broken.
-
We should also test a facet scale with a date domain. I think that was broken before I switched to InternMap.
|
"fy scale transform" is indeed broken. Example in https://observablehq.com/@observablehq/testing-facets-for-plot-1085 I wasn't able to reproduce the "date domain for fy" issue though. |
|
I expected Date domains to work since we now use InternMap, but I think the order of the domain would have been wrong before in some cases. Thanks for testing. |
|
I think we should merge this and open a new issue for the "fy scale transform" bug (which is independent)? |
|
I don't think it's a regression, just an old bug. Tests in https://observablehq.com/@observablehq/testing-facets-for-plot-1085 indicate that it is not working in any branch, nor in 0.6.0. Am I missing something? |
* define facets from each mark * allow facet: "exclude" in mark faceting this changes one unit test as it does not maintain the data order in the exclude facet. but that order was kinda arbitrary anyway. Use an explicit sort transform if necessary. * a bit more documentation * unnecessarily polish multiplication table test * stricter/looser facet validation * add TODO * remove and simplify the weird parts (j, empty facet determination) add a test plot with mark-level facets and exclude clarify comments simplify proper facet look-up for grid lines * clean up filterFacets * fix comments * regroup all facet handling, simplify * inline maybeFacet; improve backwards compatibility * minimize diff * comments * clean * fix tests * minimize diff * keep top-level facet state separate * avoid re-initializing fx and fy channels * TODO re. applyScaleTransforms * prettier * adopt InternMap; remove sorting * nullish, not undefined * style * remove TODO * fx and fy aren’t in mark.channels * remove todo * fix comment * use groups * tighten state * sparse facetsIndex * simpler facetsIndex initialization * small consolidation * compute exclude facet index earlier * simpler facet channels collection Co-authored-by: Mike Bostock <[email protected]>
Closes #1081
The internal changes are pretty substantial, but hopefully easy to read.
Note that one unit test changes because the order of the index returned by the "exclude" option is not guaranteed (it was arbitrary anyways).