explicit facet option in a mark #450
Conversation
src/mark.js
Outdated
| const names = new Set(); | ||
| this.data = data; | ||
| this.facet = facet; |
There was a problem hiding this comment.
We should do some validation and coercion here.
I also think we can unify the types a little more. Currently:
"exclude"is a string,nullandundefinedare equivalent, andtrueandfalseare booleans.
If we want to be more analogous to the scale.axis option, perhaps:
"exclude""include", andtrueis shorthand for"include"undefinedis"include"if data is strict equal, and otherwisenullto disable facetingnullor any other falsey value is mapped tonullto disable faceting
Any other truthy value, after being coerced to a string, would throw an error. We’d use the keyword helper:
Line 126 in 068f8d8
Co-authored-by: Mike Bostock <[email protected]>
- "include" (shorthand: true) - "exclude" - null (or falsy) undefined is the default test on data === facet.data.
src/facet.js
Outdated
| const markFacets = mark.facet == null ? mark.data === this.data ? facetsIndex : undefined | ||
| : mark.facet === "exclude" ? facetsIndex.map(facet => Uint32Array.from(difference(index, facet))) | ||
| : mark.facet === true ? facetsIndex | ||
| const facet = mark.facet && keyword(mark.facet, "facet", ["include", "exclude"]); |
There was a problem hiding this comment.
This (validation and type coercion of the facet option via keyword) needs to happen in the mark constructor (as I said previously), not here in initialize.
There was a problem hiding this comment.
yes I understood your comment just after pushing the previous commit 8-)
I think it's worth adding an "auto" option value (rather than undefined), which makes the code easier to read, and would maybe clarify the documentation
There was a problem hiding this comment.
Yea, I wasn’t sure whether we needed "auto" in addition to undefined, but sure, if you want.
src/facet.js
Outdated
| const facet = mark.facet && keyword(mark.facet, "facet", ["include", "exclude"]); | ||
| const markFacets = facet === undefined ? mark.data === this.data ? facetsIndex : undefined | ||
| : facet === "include" ? facetsIndex | ||
| : facet === "exclude" ? facetsIndex.map(f => Uint32Array.from(difference(index, f))) |
There was a problem hiding this comment.
I think we probably shouldn’t bother with this optimization, but: this could be extracted out of the for loop using a lazily-constructed variable so that it can be shared across multiple excluded marks.
There was a problem hiding this comment.
it's cheap so I added it too
adds an "auto" symbol as default value computes the facet exclusion index only once.
Co-authored-by: Mike Bostock <[email protected]>
…to fil/explicit-facets
|
Wow! |
you can enable or disable faceting explicitly on a mark with the facet option, which accepts the following values:
See https://observablehq.com/@fil/plot-explicit-facet-option