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

Consider dropping support for format and title in axis and legends for Vega Lite 3.0 #4057

Closed
Saba9 opened this issue Jul 19, 2018 · 13 comments
Labels
RFC / Discussion 💬 For discussing proposed changes

Comments

@Saba9
Copy link
Member

Saba9 commented Jul 19, 2018

Proposed by @domoritz. Allowing users to to define format and title in two locations seems confusing.
One example of potentially confusing behavior is that when generating tool tips we use the encoding title and format and not the axis title and format which might not be straight forward for users.

@domoritz domoritz added this to the 3.0 milestone Jul 19, 2018
@domoritz domoritz added the RFC / Discussion 💬 For discussing proposed changes label Jul 19, 2018
@kanitw
Copy link
Member

kanitw commented Jul 19, 2018

FYI, currently there is only format for text, but not other channels. So I don't know if we want a breaking changes there, especially that the format are only for axis and legend labels, but have no effect on the data fields itself.

For title, it will be also weird if there are properties for enabling parts of axis ( ticks, labels, domain, grid), but no title property. So if we gonna change, a sensible change would be changing title to be boolean (so it's more consistent with ticks, labels, grid, domain).

@kanitw
Copy link
Member

kanitw commented Jul 19, 2018

For title, it will be also weird if there are properties for enabling parts of axis ( ticks, labels, domain, grid), but no title property. So if we gonna change, a sensible change would be changing title to be boolean (so it's more consistent with ticks, labels, grid, domain).

Doing so might be useful if we want to enable parts of axis to show both top and bottom/left and right by making the ticks, labels, domain, title support boolean | 'top' | 'bottom' | 'both' | 'left' | 'right' or something. (This is a new idea that I was thinking when I'm thinking about having labels / ticks on both side of the plots. -- I'm not sure if this is the best idea yet.)

@domoritz
Copy link
Member

I'm not so sure about positioning titles that way but do you agree that we should change title to only support booleans?

@domoritz
Copy link
Member

Any objections @arvind @jheer?

@arvind
Copy link
Member

arvind commented Jul 20, 2018

Changing the axis/legend title property to boolean makes sense to me!

@kanitw
Copy link
Member

kanitw commented Jul 20, 2018

I don't know if we should really introduce this breaking changes given it's probably already widely used. In fact, we even use axis.title in the figures for Making Data Visual ourselves!

While we can of course go update the online gallery for making data visual, it seems like we don't gain much from this breaking change -- maybe a better thing to do is fix the bug in tooltip encoding to read axis/legend/header title too?

@domoritz
Copy link
Member

I think it's better to have only one way to set the title, rather than four (title, axis title, legend title, header title). We only used axis.title because that was the only way. Yes, it's a breaking change but we could add a simple preprocessor that copies axis.title to title when it's not just a boolean. I think now is a good time to change this.

@kanitw
Copy link
Member

kanitw commented Jul 20, 2018

Yes, it's a breaking change but we could add a simple preprocessor that copies axis.title to title when it's not just a boolean. I think now is a good time to change this.

If you're adding a pre-processor, then it's not changing the behavior then. Just how we implement it.

@domoritz
Copy link
Member

Well, and I'd not document it officially. Just to not annoy users too much.

@kanitw
Copy link
Member

kanitw commented Jul 20, 2018

Well, and I'd not document it officially. Just to not annoy users too much.

I don't see how it's more annoying for users. Having document that doesn't match behavior is weird. (I do agree that having two ways of doing things is actually more annoying for us who maintain the projects.)

Thinking more, it's also kinda weird that they can specify title outside but then disable it in axis. In a way, if they don't want a title in a first place, they could just specify title = '' from the outside.

If we start from scratch, I can see the argument for having only one of this. But at this point, removing it isn't really beneficial and will only break things and annoy users. So let's only discuss how we gonna refactor this internal and prevent future errors like we have with tooltip.

I think we should think about fieldDef.title as a shorthand and axis/legend/header.title as the full form (as we will merge axis/legend/header.title in our composition logic).
Basically, encoding on full spec can have fieldDef.title, but internal encoding shouldn't.
Otherwise, it's kinda weird that we normalize axis/legend/header.title to fieldDef.title only to later copy fieldDef.title back to axis/legend/header.title` inside.

@domoritz
Copy link
Member

Thinking more, it's also kinda weird that they can specify title outside but then disable it in axis. In a way, if they don't want a title in a first place, they could just specify title = '' from the outside.

No, you could have a title that is used for tooltips but not for the axis. Or it's only visible if you use the channel for a legend.

@kanitw
Copy link
Member

kanitw commented Jul 20, 2018

Or it's only visible if you use the channel for a legend.

I agree with this part.

No, you could have a title that is used for tooltips but not for the axis.

Not sure how important is this case, I can see your point.
If we want to support this, we have two options:

  1. normalize to fieldDef.title (but it will be weird like I said)
  2. just fix the bug for tooltip

I think 2) is way less work and doesn't make the code more complicated. (It's complicated, but I don't see 1) being simpler.)

@kanitw
Copy link
Member

kanitw commented Jul 26, 2018

We fixed the tooltip bug in #4076 and we agreed in person that we should not change the syntax, so let's close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC / Discussion 💬 For discussing proposed changes
Projects
None yet
Development

No branches or pull requests

4 participants