fix: Init the better orient for the ranged bar #8475

yhoonkim · 2022-10-12T00:49:10Z

Update the init logic to assign the better orient for ranged bar charts. The new logic is the same as the area.

Before

X, Y, Y2 -> "horizontal"
Y, X, X2 -> "vertical"

After

X, Y, Y2 -> Y, Y2 are binned ? "horizontal" : "vertical"
Y, X, X2 -> X, X2 are binned ? "vertical" : "horizontal"

📦 Published PR as canary version: 5.5.1--canary.8475.ccc3007.0

✨ Test out this PR locally via:

npm install vega-lite@5.5.1--canary.8475.ccc3007.0
# or 
yarn add vega-lite@5.5.1--canary.8475.ccc3007.0

yhoonkim · 2022-10-12T00:50:57Z

test/compile/mark/bar.test.ts

@@ -43,9 +43,9 @@ describe('Mark: Bar', () => {
    expect(props.height).toBeUndefined();
  });

-  it('should draw vertical bar, with y from zero to field value and bar with quantitative x, x2, and y', () => {
-    const x: PositionFieldDef<string> = {field: 'bin_start', type: 'quantitative'};


Since there are the test cases for the bin, I change this to cover not-binned cases.

The new tests are fine, but shouldn't you keep the old tests but add bin: 'binned' to it to cover the

if (isFieldDef(y) && isBinned(y.bin)) { condition you modified above?

kanitw

Please take a look at regressions and resolve unintended changes + revise the tests you removed.

kanitw · 2022-10-12T20:16:04Z

test/compile/mark/bar.test.ts

@@ -43,9 +43,9 @@ describe('Mark: Bar', () => {
    expect(props.height).toBeUndefined();
  });

-  it('should draw vertical bar, with y from zero to field value and bar with quantitative x, x2, and y', () => {
-    const x: PositionFieldDef<string> = {field: 'bin_start', type: 'quantitative'};


The new tests are fine, but shouldn't you keep the old tests but add bin: 'binned' to it to cover the

if (isFieldDef(y) && isBinned(y.bin)) { condition you modified above?

kanitw · 2022-10-12T20:16:58Z

examples/compiled/histogram_nonlinear.vg.json

-          "y": {"scale": "y", "field": "residency_end"},
-          "y2": {"scale": "y", "field": "residency_start"}
+          "yc": {"scale": "y", "field": "residency"},
+          "height": {"value": 5}


This diff seems wrong?

You may need to add bin: "binned" in the related VL spec.

kanitw · 2022-10-12T20:17:46Z

examples/compiled/layer_cumulative_histogram.vg.json

-          "y": {"scale": "y", "field": "count"},
-          "y2": {"scale": "y", "value": 0}
+          "yc": {"scale": "y", "field": "count"},
+          "height": {"value": 5}


This diff also seems wrong. You may need to add bin: "binned" in the related VL spec.

kanitw · 2022-10-12T20:18:17Z

examples/compiled/wheat_wages.vg.json

@@ -142,7 +142,7 @@
          },
          "x": {"scale": "x", "field": "year"},
          "y": {"scale": "y", "field": "wages"},
-          "y2": {"scale": "y", "value": 0},
+          "y2": {"field": {"group": "height"}},


This diff seems wrong. Not sure why, please investigate / debug to prevent this regression.

examples/specs/layer_cumulative_histogram.vl.json

yhoonkim · 2022-10-12T23:07:19Z

examples/specs/histogram_nonlinear.vl.json

@@ -48,7 +48,7 @@
  "title": "Distribution of Frame Render Time (ms)",
  "width": {"step": 40},
  "height": 100,
-  "mark": {"type": "bar", "cornerRadiusEnd": 0},
+  "mark": {"type": "bar", "cornerRadiusEnd": 0, "orient": "vertical"},


Tried to add "binned" but failed due to #8476.
Adding "orient" is also an alternative way to make this spec work.

examples/specs/normalized/layer_cumulative_histogram_normalized.vl.json

kanitw · 2022-10-12T23:13:09Z

examples/specs/wheat_wages.vl.json

@@ -74,7 +74,7 @@
        { "calculate": "((!datum.commonwealth && datum.index % 2) ? -1: 1) * 2 + 95", "as": "offset" },
        { "calculate": "95", "as": "y" }
      ],
-      "mark": {"type": "bar", "stroke": "#000"},
+      "mark": {"type": "bar", "stroke": "#000", "orient": "vertical"},


This one has all of y, y2, x, x2, I think it should just be a rect.

kanitw

LGTM. Please try to see if changing bar to rect in wheat_wages works before merging.

yhoonkim · 2022-10-12T23:43:53Z

examples/specs/wheat_wages.vl.json

@@ -16,7 +16,7 @@
  },
  "layer": [
    {
-      "mark": {"type": "bar", "fill": "#aaa", "stroke": "#999"},
+      "mark": {"type": "bar", "fill": "#aaa", "stroke": "#999", "orient": "vertical"},


@kanitw For this one, I will keep as "bar" but change to add "binned" on encoding.x instead of the "orient" since this spec has a kind of the bin transform implying it is binned.

vega-org-bot · 2022-10-14T16:21:33Z

🚀 PR was released in v5.6.0 🚀

* Add example for ranged bar with not binned position channels * Change the init logic to assgin better orient for ranged bar

Add example for ranged bar with not binned position channels

d8f3f63

yhoonkim commented Oct 12, 2022

View reviewed changes

yhoonkim requested a review from kanitw October 12, 2022 00:51

yhoonkim marked this pull request as ready for review October 12, 2022 00:51

kanitw requested changes Oct 12, 2022

View reviewed changes

yhoonkim force-pushed the yh/ranged-bar-orient branch from a9d1dc5 to 9ae6cf3 Compare October 12, 2022 23:02

yhoonkim commented Oct 12, 2022

View reviewed changes

examples/specs/layer_cumulative_histogram.vl.json Show resolved Hide resolved

yhoonkim commented Oct 12, 2022

View reviewed changes

examples/specs/normalized/layer_cumulative_histogram_normalized.vl.json Show resolved Hide resolved

yhoonkim requested a review from kanitw October 12, 2022 23:08

kanitw reviewed Oct 12, 2022

View reviewed changes

kanitw approved these changes Oct 12, 2022

View reviewed changes

yhoonkim commented Oct 12, 2022

View reviewed changes

Change the init logic to assgin better orient for ranged bar

ccc3007

yhoonkim force-pushed the yh/ranged-bar-orient branch from 9ae6cf3 to ccc3007 Compare October 12, 2022 23:44

yhoonkim merged commit 217e860 into next Oct 13, 2022

yhoonkim deleted the yh/ranged-bar-orient branch October 13, 2022 00:06

vega-org-bot added the prerelease label Oct 13, 2022

vega-org-bot mentioned this pull request Oct 13, 2022

Release 5.6.0 #8469

Merged

6 tasks

vega-org-bot added released This PR has been released by auto shipit and removed prerelease labels Oct 14, 2022

BradyJ27 pushed a commit to BradyJ27/vega-lite that referenced this pull request Oct 19, 2023

fix: Init the better orient for the ranged bar (vega#8475)

ae458f5

* Add example for ranged bar with not binned position channels * Change the init logic to assgin better orient for ranged bar

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Init the better orient for the ranged bar #8475

fix: Init the better orient for the ranged bar #8475

yhoonkim commented Oct 12, 2022 •

edited by vega-org-bot

yhoonkim Oct 12, 2022

kanitw Oct 12, 2022

kanitw left a comment

kanitw Oct 12, 2022

kanitw Oct 12, 2022

kanitw Oct 12, 2022

kanitw Oct 12, 2022 •

edited

yhoonkim Oct 12, 2022

kanitw Oct 12, 2022

yhoonkim Oct 12, 2022

kanitw left a comment

yhoonkim Oct 12, 2022

vega-org-bot commented Oct 14, 2022

fix: Init the better orient for the ranged bar #8475

fix: Init the better orient for the ranged bar #8475

Conversation

yhoonkim commented Oct 12, 2022 • edited by vega-org-bot

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanitw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanitw Oct 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanitw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vega-org-bot commented Oct 14, 2022

yhoonkim commented Oct 12, 2022 •

edited by vega-org-bot

kanitw Oct 12, 2022 •

edited