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.

Sign up for GitHub

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

text, background, and chart colors wired in #382

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions packages/cedar-amcharts/src/render/render.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"

const styles = definition.styles
// If a backgroundColor..
if (styles.backgroundColor) { spec.backgroundColor = styles.backgroundColor }
// If a backgroundAlpha..
if (styles.backgroundAlpha) { spec.backgroundAlpha = styles.backgroundAlpha }
// If a textColor
if (styles.textColor) { spec.color = styles.textColor }
}

// Iterate over datasets
definition.datasets.forEach((dataset, d) => {
Expand All @@ -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.

graph.lineColor = definition.styles.colors[s]
}

graph.balloonText = `${graph.title} [[${spec.categoryField}]]: <b>[[${graph.valueField}]]</b>`

// Group vs. stack
Expand Down
18 changes: 16 additions & 2 deletions packages/cedar/src/Chart.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,14 @@ export interface ISeries {
}

export interface ILegend {
enable: boolean
enable?: boolean
}

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......

colors?: string[],
textColor?: string
}

export interface IDefinition {
Expand All @@ -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 default class Chart {
Expand Down Expand Up @@ -135,6 +143,12 @@ export default class Chart {
return this._definitionAccessor('legend', newLegend)
}

public styles(newStyles: IStyles): Chart
public styles(): IStyles
public styles(newStyles?: any): any {
return this._definitionAccessor('styles', newStyles)
}

// data is read only
public data() {
return this._data
Expand Down