gosling.js icon indicating copy to clipboard operation
gosling.js copied to clipboard

Group channels as `encoding`

Open sehilyi opened this issue 2 years ago • 1 comments

... FWIW, I don't think visibility exactly belong in .encode(...) and rather .properties would be more appropriate usage currently.

The ambiguity arises because encoding fields (for simplicity, fields with type Channel) are not nested under a field themselves but alongside "mark", "style", "dataTransforms", visibility, etc.

For this reason, .encode(...) must accept any key-word argument. For example, you could equivalently pass width, height, data to .encode as well as any gos.Channel to .properties(...).

I worry that this could become confusing for users, but adding this additional mixin should certainly prevent users from specifying visibility in .encode or .properties.

As a side note, if gosling were to group channels within a key (e.g. "channel" or "encoding"), then it would be easier to restrict the usage.

{
  data: { ... },
  width: { ... },
  height: { ... },
  mark: "bar",
  channels: { x: ..., y: ... },
  visibility: [ ... ],
  dataTransform: [ ... ],
}

In this case, you can't set x directly in gos.Track(...) or gos.Track(...).properties(...). I know this would be a somewhat substantial schema change, but something worth considering in my opinion.

Originally posted by @manzt in https://github.com/manzt/gos/issues/34#issuecomment-904798525

Recently, I actually also ran into an issue where I can get benefit from grouping channels (e.g., encoding: { x... }). Also, the only reason for not grouping channels I think was to avoid deeper hierarchy, but I don't think this is a critical issue here.

I think I can change the schema (and other related codes and examples) in the Gosling and open a PR for this.

Originally posted by @sehilyi in https://github.com/manzt/gos/issues/34#issuecomment-904857604

sehilyi avatar Aug 27 '21 13:08 sehilyi

Hmm, this seems to be a much more substantial update than I expected. Would need to spend some time later.

sehilyi avatar Aug 27 '21 17:08 sehilyi