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

Change pie chart color #14

Closed
hypolas opened this issue Oct 30, 2024 · 5 comments · Fixed by #15
Closed

Change pie chart color #14

hypolas opened this issue Oct 30, 2024 · 5 comments · Fixed by #15
Assignees
Labels
bug Something isn't working

Comments

@hypolas
Copy link

hypolas commented Oct 30, 2024

Hello,

I try to set color to pie chart serie.

In my case, I need, for exemple, to set color "red" (for error data) and green (for valide data) .

That seems don't work.  I try with:

	p, err := charts.PieRender(
		values,
		charts.TitleOptionFunc(charts.TitleOption{
			Text:    "Rainfall vs Evaporation",
			Subtext: "Fake Data",
			Left:    charts.PositionCenter,
		}),
		charts.PaddingOptionFunc(charts.Box{
			Top:    20,
			Right:  20,
			Bottom: 10,
			Left:   20,
		}),
		charts.LegendOptionFunc(charts.LegendOption{
			Orient: charts.OrientVertical,
			Data: []string{
				"tests",
				"failures",
				"errors",
			},
			Left: charts.PositionLeft,
		}),
		charts.PieSeriesShowLabel(),
		func(opt *charts.ChartOption) {
			opt.SeriesList[1].Style.FillColor = drawing.ColorRed
			opt.SeriesList[2].Style.FillColor = drawing.ColorRed
		},
	)
@jentfoo jentfoo added the bug Something isn't working label Oct 30, 2024
@jentfoo jentfoo self-assigned this Oct 30, 2024
@jentfoo
Copy link
Member

jentfoo commented Oct 30, 2024

Thank you for opening this issue @hypolas. This fork has been re-evaluating the chart configuration options and the series Style has not yet been considered.

Looking at the implementation, it looks like setting the FillColor currently only functions on Bar charts. This could be easily extended to also support Pie Charts, but there ends up being a weird side effect that the Legend does not have a reference to the series (also true for bar charts), and thus it's colors are not updated, for example:
TestPieChartSeriesStyleFill-actual-2925364196

One goal of this fork is to make themes more straight forward. We don't require themes to be globally registered (they are passed in with the chart configuration). Would a suitable alternative be to take a theme you like, and then replace the SeriesColors with the colors you want represented? I still want to make theme mutations easier, it would look something like this:

	baseTheme := charts.GetTheme(ThemeVividLight)
	var seriesColors []charts.Color
	for i := range series {
		if red {
			seriesColors = append(seriesColors, drawing.ColorRed)
		} else {
			seriesColors = append(seriesColors, baseTheme.GetSeriesColor(i))
		}
	}
	customTheme := baseTheme.WithSeriesColors(seriesColors)
	// or possibly
	customTheme := baseTheme.WithOptions(ThemeOption{SeriesColors: seriesColors})

Then your pie chart would be constructed like this:

	p, err := charts.PieRender(
		values,
		charts.ThemeOptionFunc(customTheme),
		charts.TitleOptionFunc(charts.TitleOption{
			Text:    "Rainfall vs Evaporation",
			Subtext: "Fake Data",
			Left:    charts.PositionCenter,
		}),
		charts.PaddingOptionFunc(charts.Box{
			Top:    20,
			Right:  20,
			Bottom: 10,
			Left:   20,
		}),
		charts.LegendOptionFunc(charts.LegendOption{
			Orient: charts.OrientVertical,
			Data: []string{
				"tests",
				"failures",
				"errors",
			},
			Left: charts.PositionLeft,
		}),
		charts.PieSeriesShowLabel(),
	)

I believe that having a single method of setting the theme for the series would be more straight forward, but I would appreciate your feedback before we make any changes. Thank you!

jentfoo added a commit that referenced this issue Oct 31, 2024
This change removes the ability to configure the styling at the series level.

As seen in this change and discussed in issue #14, these style options were not very broadly recognized.

Instead the goal is to make theme customization easier. As a potential replacement to this configuration the Themes can now accept series and background color changes.

If we do decide that we want to bring back per-series styling, I think we should make those top level fields so that we only introduce fiels which are broadly recognized.
@jentfoo
Copy link
Member

jentfoo commented Oct 31, 2024

@hypolas, I just submitted a PR which removes the series Style you were configuring and instead introduces the ergonomics around changing the series colors of the theme: #15

Let me know your thoughts on moving forward with a change like this, or if you think there is a good reason to leave this style configuration at the series level. Thank you!

@hypolas
Copy link
Author

hypolas commented Oct 31, 2024

Hello.

Than you for your very fast answer.

For solve my problem quikly, I had modified my code like this :

func (g Graph) buildGraph() {
	p, err := charts.PieRender(
		g.Values,
		g.makeDefaultTheme(),
		charts.TitleOptionFunc(charts.TitleOption{
			Text:    g.Text,
			Subtext: g.Subtext,
			Left:    charts.PositionCenter,
		}),
		charts.PaddingOptionFunc(charts.Box{
			Top:    20,
			Right:  20,
			Bottom: 20,
			Left:   20,
		}),
		charts.LegendOptionFunc(charts.LegendOption{
			Orient: charts.OrientVertical,
			Data:   g.Legends,
			Top:    "5%",
			Left:   charts.PositionLeft,
		}),
		charts.PieSeriesShowLabel(),
	)

	if err != nil {
		panic(err)
	}

	if buf, err := p.Bytes(); err != nil {
		panic(err)
	} else if err = writeFile(buf); err != nil {
		panic(err)
	}
}

func (g Graph) makeDefaultTheme() charts.OptionFunc {
	th := charts.ThemeOptionFunc(charts.MakeTheme(charts.ThemeOption{
		SeriesColors:    g.Colors,
		TextColor:       drawing.ColorFromHex("555555"),
		BackgroundColor: drawing.ColorWhite,
	}))

	return th
}

g.Color is a slice of color previously generated with this code.

func colorSwitch(result string) drawing.Color {
	switch result {
	case "failed":
		return drawing.ColorRed
	case "failure":
		return drawing.ColorRed
	case "passed":
		return drawing.ColorLime
	case "ok":
		return drawing.ColorLime
	case "errors":
		colorError := drawing.Color{
			R: 244,
			G: 100,
			B: 100,
			A: 255,
		}

		return colorError
	}
	return drawing.ColorWhite
}

pie-chart

If this can help somebody ;)

@jentfoo
Copy link
Member

jentfoo commented Nov 1, 2024

Thank you for the feedback @hypolas, I am glad to hear that the custom theme options is a workable solution for you.

I went ahead and polished PR #15 further, making sure that Font configuration (which is broadly used and common) is encapsulated in the FontStyle struct instead of needing the broader Style struct (which could lead to similar confusion as you had here). Feel free to give the PR a look if you have interest, I would appreciate any feedback you might have.

Once merged I plan to release this under version 0.2.0. Although you have a good solution here, the primary benefit to you will be the WithSeriesColors which could allow you to easily only change the series colors.

Let me know if you have questions or thoughts, thank you again!

@jentfoo
Copy link
Member

jentfoo commented Nov 5, 2024

These changes were just pushed under the v0.2.0 tag. Let me know if you have any questions or further recommendations. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants