feat: support offset channel for grouped bar chart and jittering #7684
Conversation
1e68335 to
f544a4f
Compare
d60c72f to
ee0d03a
Compare
…order This is required for #7684 as xOffset must be normalized after x (same for y)
…order This is required for #7684 as xOffset must be normalized after x (same for y)
src/compile/scale/range.ts
Outdated
| // otherwise use the position | ||
| return [0, {signal: `bandwidth('${positionScaleName}')`}]; | ||
| } else { // continuous scale | ||
| throw new Error(`Cannot use ${channel} scale if ${positionChannel} scale is not discrete.`) |
There was a problem hiding this comment.
We should never reach this line because we won't even generate the scale in the first place.
src/scale.ts
Outdated
| * @minimum 0 | ||
| * @maximum 1 | ||
| */ | ||
| nestedOffsetPaddingInner?: number | ES; |
There was a problem hiding this comment.
Any opinion about this config name?
There was a problem hiding this comment.
Why is it nested and not just offset padding?
There was a problem hiding this comment.
renamed to bandWithNestedOffsetPadding --> This is padding of the x/y scale, when they have x/yOffset nested, but the padding is for x/y scales, not x/yOffset scales. (offsetPaddingInner/Outer is then for x/yOffset scales)
| /** | ||
| * Whether to apply the step to position scale or offset scale when there are both `x` and `xOffset` or both `y` and `yOffset` encodings. | ||
| */ | ||
| for?: StepFor; |
There was a problem hiding this comment.
Any opinion about this property name?
There was a problem hiding this comment.
forScale or applyTo or along maybe?
There was a problem hiding this comment.
I think
{step: 20, for: "position"}
reads better than
{step: 20, forScale/applyTo/along: "position"}?
There was a problem hiding this comment.
Full examples
{
width: {step: 23, for: 'position'},
mark: 'bar',
encoding: {
x: {field: 'x', type: 'nominal'},
xOffset: {field: 'subx', type: 'nominal'}
}
}{
width: {step: 23, for: 'offset'},
mark: 'bar',
encoding: {
x: {field: 'x', type: 'nominal'},
xOffset: {field: 'subx', type: 'nominal'}
}
}Another option may be forChannel. I am okay with for.
There was a problem hiding this comment.
I think for is also better than forChannel because it's for position (which is a category of channel). "for channel position" is swapping the order of the words anyway.
If it's unclear if a longer word is clearer, I generally prefer brevity
f7064cb to
7ae3ea5
Compare
7ae3ea5 to
7a6836b
Compare
747ad6e to
e65b223
Compare
There was a problem hiding this comment.
- Offset doesn't support title. Should we remove it from the schema?
- Shouldn't the grouped bar chart have some padding between the bars? It looks pretty tight right now. Also, how do you add padding and still make sure that bars are centered?
Other than that, this is awesome!
src/stack.ts
Outdated
| let dimensionDef = encoding[dimensionChannel]; | ||
| const dimensionChannel: 'x' | 'y' | 'theta' | 'radius' = getDimensionChannel(fieldChannel); | ||
| const groupbyChannels: StackProperties['groupbyChannels'] = []; | ||
| const groupbyFields: FieldName[] = []; |
There was a problem hiding this comment.
Could we make this a Set<FieldName>?
| /** | ||
| * Whether to apply the step to position scale or offset scale when there are both `x` and `xOffset` or both `y` and `yOffset` encodings. | ||
| */ | ||
| for?: StepFor; |
There was a problem hiding this comment.
forScale or applyTo or along maybe?
There is no guide, but the field itself still has title, which can be used for tooltip. |
That's sort of intentional. If you google "grouped bar", even for D3, it seems like the norm is to have near-zero padding. If you strongly think we should change this, let me know. Otherwise, I think we can merge this. In any case, I also added
Good question. You can add |
Wow, that's surprising. I feel some padding is better since the bars show different categories. But I guess not padding makes the grouping clearer. I don't feel strongly about it so we can leave as it is as long as it's easy to add padding. |
|
I tested a couple specs locally and padding works like expected now. This is awesome. |
|
I'm not sure how the release cycle works on vega-lite, but it would be great if you could publish a release with this included! |
|
Yes, I'm fixing one more bug that's relevant to this and then we'll release soon. |
Unit test coverage
Add to example gallery
update docs
test examples / corner cases
bar_grouped.vl.jsonbar_grouped_fixed_width.vl.jsonbar_grouped_step_for_offset.vl.jsonbar_grouped_step_for_position.vl.jsonpoint_groupedbar_grouped_label,layer_bar_circle_groupedpoint_offset_randomayer_bar_grouped_line_ungrouped.vl.jsonbar_x_offset_without_x_broken.vl.jsonbar_grouped_repeated.vl.json