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

Add .visibility_condition mixin methods to gos.Track #34

Closed
sehilyi opened this issue Aug 23, 2021 · 2 comments
Closed

Add .visibility_condition mixin methods to gos.Track #34

sehilyi opened this issue Aug 23, 2021 · 2 comments

Comments

@sehilyi
Copy link
Member

sehilyi commented Aug 23, 2021

Before Update:

gos.Track(data).mark_text().encode(...,
  visibility=[
      gos.VisibilityCondition(operation="LT", measure="width", threshold="|xe-x|", transitionPadding=30, target="mark"),
      gos.VisibilityCondition(operation="LT", measure="zoomLevel", threshold=10, target="track")
  ]
)

After Update:

gos.Track(data).mark_text().encode(...).visibility_condition(
   operation="LT", measure="width", threshold="|xe-x|", transitionPadding=30, target="mark")
).visibility_condition(
   operation="LT", measure="zoomLevel", threshold=10, target="track"
)
@manzt
Copy link
Member

manzt commented Aug 24, 2021

I think this is a great idea! 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.

@sehilyi
Copy link
Member Author

sehilyi commented Aug 24, 2021

Thank you for sharing this!

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.

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

No branches or pull requests

2 participants