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

text, background, and chart colors wired in #382

Closed
wants to merge 1 commit into from
Closed

Conversation

benstoltz
Copy link
Member

Closes #375

@tomwayson Background color and background alpha go hand in hand. Alpha defaults to 0, meaning that if you don't set it you don't have the background color show up. I'd like to do something like: styles: { background: { color: '#xyz', alpha: 0.3 } }

Does that seem reasonable to you?

@benstoltz benstoltz requested a review from tomwayson January 12, 2018 14:04
Copy link
Member

@tomwayson tomwayson left a comment

Choose a reason for hiding this comment

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

Nicely done. A few changes.

@@ -78,6 +78,17 @@ export function fillInSpec(spec: any, definition: any) {
if (definition.legend) {
spec.legend.enabled = definition.legend.enable
}
// If we have styles....
if (definition.styles) {
// Snag out styles for brevities sake
Copy link
Member

Choose a reason for hiding this comment

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

pls refrain from using words I don't know like "brevities"

@@ -102,6 +113,11 @@ export function fillInSpec(spec: any, definition: any) {
/* tslint:enable */
// TODO: map other fields besides value like color, size, etc

// handle le color
if (definition.styles && definition.styles.colors) {
Copy link
Member

Choose a reason for hiding this comment

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

I wasn't expecting this to be needed so I'm curious what happens if you don't have this? is this for bar graph outlines? or needed for line charts?

Copy link
Member Author

Choose a reason for hiding this comment

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

graph.lineColor actually hits all the colors that are needed, both the bar fill and outline. It's also applicable to chart types like line, bubble, etc. The other option that we discussed...I'm trying to remember it....only was applicable to the fill color, not outline on bar charts for example.

@@ -67,7 +74,8 @@ export interface IDefinition {
type?: string
specification?: {}
overrides?: {},
legend?: ILegend
legend?: ILegend,
styles?: IStyles
Copy link
Member

Choose a reason for hiding this comment

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

I had planned to call this style but I think you're right to call it styles so it doesn't get confused w/ CSS.


export interface IStyles {
backgroundColor?: string,
backgroundAlpha?: number,
Copy link
Member

Choose a reason for hiding this comment

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

I prefer backgroundOpacity if that's what this really is. re: background: { color: 'red', opacity: 0.5 } that's fine, but also allow background: 'red'. Either way, you'll need a new interface like IColorOpacity and do what I'm asking this would be background: string || IColorOpacity.

What I don't like about that is that it's inconsistent w/ textColor. So unless there's a reason to make text overloaded (like we might later add font size or something), I'd say leave them as is (except changing Alpha to Opacity.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, that is a good point. Realistically textColor should become text: { color: 'red', fontWhatever: 'blahblah' } that's if we are thinking forwards a bit.

backgroundColor and backgroundOpacity could become background: { color, opacity } if we wanted it too. I don't think there would be any other additional things to add here. That being said: Background color. You should set backgroundAlpha to >0 value in order background to be visible. We recommend setting background color directly on a chart's DIV instead of using this property. < is part of the docs on the site. So maybe I should rework how this is implemented......

@tomwayson
Copy link
Member

as discussed today, before settling on the final JSON API we should see how the same settings would be set in vega-lite, Pro, and Ops Dashboard.

@benstoltz
Copy link
Member Author

benstoltz commented Jan 25, 2018

So there are several ways this could fly in Vega-Lite:

{
  "$schema": "https://vega.github.io/schema/vega-lite/v2.json",
  "description": "Stock prices of 5 Tech Companies over Time.",
  "data": {"url": "data/stocks.csv"},
  "mark": "line",
  "encoding": {
    "x": {"field": "date", "type": "temporal", "axis": {"format": "%Y"}},
    "y": {"field": "price", "type": "quantitative"},
    "color": {"field": "symbol", "type": "nominal"}
  }
}

Encoding would work for one: https://vega.github.io/vega-lite/docs/encoding.html#mark-prop

Another option would be via the mark def object: https://vega.github.io/vega-lite/docs/mark.html#mark-def More specifically the style property. It's unclear to me at least at first glance as to which would be the preferable option to set internally.

I think the biggest point of difficulty here is that it seems for VL that the notion of color/other styles would be far more tightly coupled with the notion of series than it would be for AmCharts.

Thoughts @tomwayson?

@benstoltz
Copy link
Member Author

Actually re AmCharts, there are ways to set colors differently than I have done that would be more tightly coupled with the series. I think we'd need to see how ops dashboard and pro does it, but if they similarly are fairly tightly coupled then maybe we should rework some of the style related props out of styles and into series...

@tomwayson
Copy link
Member

Once we agree on the API for colors, etc, we will need to rebase this after #423

@benstoltz
Copy link
Member Author

Honestly it might make sense to just start over from scratch. The api changes will be slightly annoying to rebase and the actual functional differences are minimal enough that starting over will be simple.

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.

2 participants