Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Facet generates customized grid #1367

Merged
merged 4 commits into from
May 22, 2016
Merged

Facet generates customized grid #1367

merged 4 commits into from
May 22, 2016

Conversation

YuhanLu
Copy link
Contributor

@YuhanLu YuhanLu commented May 18, 2016

Fix the second part of #1237.
Now the spec below generate the right output.

{
  "data": {
    "values": [
      {"a": "A","b": 28,"c": "x"}, {"a": "B","b": 55,"c": "x"}, {"a": "C","b": 43,"c": "x"},
      {"a": "A","b": 28,"c": "y"}, {"a": "B","b": 55,"c": "y"}, {"a": "C","b": 43,"c": "y"}
    ]
  },
  "mark": "square",
  "encoding": {
    "column": {
      "field": "c", 
      "type": "ordinal"
    },
    "x": {
      "field": "a", 
      "type": "ordinal",
      "axis": {
        "grid": true,
        "layer": "front",
        "properties": {
          "labels": {
            "fill": { "value": "red" }
          },
          "grid": {
            "stroke": { "value": "red" },
            "strokeWidth": { "value": 5 }
          }
        }
      }
    },
    "y": {
      "field": "b", 
      "type": "quantitative",
      "axis": {
        "grid": true,
        "layer": "front",
        "properties": {
          "labels": {
            "fill": { "value": "green" }
          },
          "grid": {
            "stroke": { "value": "green" },
            "strokeWidth": { "value": 5 }
          }
        }
      }
    }
  }
}

vega 3


// it might have more to be included here.
['grid'].forEach(function(group) {
const value = properties[group] ?
Copy link
Contributor Author

@YuhanLu YuhanLu May 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure about if there's more properties need to be added here so I leave here as a list.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I would examine properties of 'axis', 'labels', 'title', 'ticks', 'majorTicks', 'minorTicks' and see if they have similar problem.

Copy link
Contributor Author

@YuhanLu YuhanLu May 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried and grid is only the special rule here. Should I remove the list and only leave grid here or keep it as a list?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's leave it as is. Currently innerAxis is only for grid, but we can support others in the future.
(That's beyond scope of this PR.)

@YuhanLu
Copy link
Contributor Author

YuhanLu commented May 18, 2016

The problem of this bug before the change in axis.ts, the vega-lite spec will generate this vega-spec

{
          "name": "cell",
          "type": "group",
          "from": {
            "data": "source",
            "transform": [{"type": "facet","groupby": ["c"]}]
          },
          "properties": {
            "update": {
              "x": {"scale": "column","field": "c","offset": 8},
              "y": {"value": 8},
              "width": {"field": {"parent": "child_width"}},
              "height": {"field": {"parent": "child_height"}},
              "stroke": {"value": "#ccc"},
              "strokeWidth": {"value": 1}
            }
          },
          "marks": [
            {
              "name": "child_marks",
              "type": "symbol",
              "properties": {
                "update": {
                  "x": {"scale": "x","field": "a"},
                  "y": {"scale": "y","field": "b"},
                  "size": {"value": 30},
                  "shape": {"value": "square"},
                  "opacity": {"value": 0.7},
                  "fill": {"value": "#4682b4"}
                }
              }
            }
          ],
          "axes": [
            {
              "type": "x",
              "scale": "x",
              "grid": true,
              "tickSize": 0,
              "properties": {
                "labels": {"text": {"value": ""}},
                "axis": {"stroke": {"value": "transparent"}}
              },
              "layer": "front",
              "ticks": 5
            },
            {
              "type": "y",
              "scale": "y",
              "grid": true,
              "tickSize": 0,
              "properties": {
                "labels": {"text": {"value": ""}},
                "axis": {"stroke": {"value": "transparent"}}
              },
              "layer": "front"
            }
          ]
        }

And not it will look like:

{
          "name": "cell",
          "type": "group",
          "from": {
            "data": "source",
            "transform": [{"type": "facet","groupby": ["c"]}]
          },
          "properties": {
            "update": {
              "x": {"scale": "column","field": "c","offset": 8},
              "y": {"value": 8},
              "width": {"field": {"parent": "child_width"}},
              "height": {"field": {"parent": "child_height"}},
              "stroke": {"value": "#ccc"},
              "strokeWidth": {"value": 1}
            }
          },
          "marks": [
            {
              "name": "child_marks",
              "type": "symbol",
              "properties": {
                "update": {
                  "x": {"scale": "x","field": "a"},
                  "y": {"scale": "y","field": "b"},
                  "size": {"value": 30},
                  "shape": {"value": "square"},
                  "opacity": {"value": 0.7},
                  "fill": {"value": "#4682b4"}
                }
              }
            }
          ],
          "axes": [
            {
              "type": "x",
              "scale": "x",
              "grid": true,
              "tickSize": 0,
              "properties": {
                "labels": {"text": {"value": ""}},
                "axis": {"stroke": {"value": "transparent"}},
                "grid": {
                  "stroke": {"value": "red"},
                  "strokeWidth": {"value": 5}
                }
              },
              "layer": "front",
              "ticks": 5
            },
            {
              "type": "y",
              "scale": "y",
              "grid": true,
              "tickSize": 0,
              "properties": {
                "labels": {"text": {"value": ""}},
                "axis": {"stroke": {"value": "transparent"}},
                "grid": {
                  "stroke": {"value": "green"},
                  "strokeWidth": {"value": 5}
                }
              },
              "layer": "front"
            }
          ]
        }

The difference is from child.axis, which generated by axis.ts parseInnerAxis(). parseInnerAxis() didn't update child.axis.properties.grid and not I add a parse for properties.grid so the under vega spec mark cell, it will include grid.

@YuhanLu YuhanLu changed the title Facet generate customized grid Facet generates customized grid May 18, 2016
@YuhanLu
Copy link
Contributor Author

YuhanLu commented May 19, 2016

Hmm actually it works for the old version of axis properties but not the new version. It cannot parse axisColor: green to be axis: {stroke: {value: green}}. I will fix it.

@YuhanLu YuhanLu changed the title Facet generates customized grid [WIP] Facet generates customized grid May 19, 2016
@YuhanLu YuhanLu changed the title [WIP] Facet generates customized grid Facet generates customized grid May 19, 2016
@YuhanLu
Copy link
Contributor Author

YuhanLu commented May 19, 2016

It was my mistake and it works for new version of axis properties, too.

The spec below using the new axis properties will generate the same output as above.

{
  "data": {
    "values": [
      {"a": "A","b": 28,"c": "x"}, {"a": "B","b": 55,"c": "x"}, {"a": "C","b": 43,"c": "x"},
      {"a": "A","b": 28,"c": "y"}, {"a": "B","b": 55,"c": "y"}, {"a": "C","b": 43,"c": "y"}
    ]
  },
  "mark": "square",
  "encoding": {
    "column": {
      "field": "c", 
      "type": "ordinal"
    },
    "x": {
      "field": "a", 
      "type": "ordinal",
      "axis": {
        "grid": true,
        "layer": "front",
        "gridColor": "red",
        "gridWidth": 5,
        "properties": {
          "labels": {
            "fill": { "value": "red" }
          }
        }
      }
    },
    "y": {
      "field": "b", 
      "type": "quantitative",
      "axis": {
        "grid": true,
        "layer": "front",
        "gridColor": "green",
        "gridWidth": 5,
        "properties": {
          "labels": {
            "fill": { "value": "green" }
          }
        }
      }
    }
  }
}

@kanitw
Copy link
Member

kanitw commented May 20, 2016

Haven't got a chance to look deeply, but could you add unit test first?

@YuhanLu
Copy link
Contributor Author

YuhanLu commented May 20, 2016

Unit test is added.

@kanitw kanitw merged commit 9875a2f into master May 22, 2016
@kanitw kanitw deleted the zl/facet-axis-ignore branch May 22, 2016 04:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants