terezka / elm-charts

Create SVG charts in Elm.
https://www.elm-charts.org
BSD 3-Clause "New" or "Revised" License
698 stars 67 forks source link

Inverted legends when using stacked barcharts #101

Open n1k0 opened 2 years ago

n1k0 commented 2 years ago

Hello,

First, thank you so much for this great library 👍

I've stumbled upon what I think is a bug when using stacked bar charts and legends combined, as demoed in the short code sample below:

import Chart as C
import Chart.Attributes as CA
import Html exposing (Html)

chart : Html msg
chart =
    C.chart
        [ CA.height 500, CA.width 500 ]
        [ C.legendsAt .min .max [] []
        , [ { x = 1, y = 2, z = 3 }
          , { x = 4, y = 5, z = 6 }
          , { x = 7, y = 8, z = 9 }
          ]
            |> C.bars []
                [ C.stacked
                    [ C.bar .x []
                        |> C.named "X"
                    , C.bar .y []
                        |> C.named "Y"
                    , C.bar .z []
                        |> C.named "Z"
                    ]
                ]
        , C.barLabels [ CA.moveDown 16, CA.color "white" ]
        ]

This gives this:

image

As you can see, the legends don't match the data and their graphical representation. It seems to me that the legend colors are reversed. Or am I missing a piece of understanding on how stacked barcharts and legends should work together?

Thanks again for your amazing library!

Enkidatron commented 1 year ago

I also encountered this.

I actually think that it is the colors in the actual chart that are reversed, and not the colors in the legend. For other chart types (non-stacked bar, stacked area, line, etc), the first series/bar is purple, then pink, then light blue. Since the .x bar is the first bar that is defined, it should be purple. The legend accurately shows it as purple, but the chart itself shows it in light blue.

I have found that specifying colors for each bar in a stacked bar does work correctly, it is only the default colors that are reversed.

EDIT: I have now done research:

I believe that this is caused by Line 170 of src/Internal/Produce.elm, which reverses the stacks before mapping them into toSeriesItem. I am not sure why these stacks need to be reversed at all, but I believe current behavior could be maintained (except for the default colors, which would be fixed) by using List.reverse after the List.indexedMap instead of before (this index is named colorIndex, and it appears to only be used for the default colors). I am working on some local tests that I can use to confirm this before I put together a pull request.

Enkidatron commented 1 year ago

On further research, my above belief is close but flawed. Moving the List.reverse around inside of Line 170 will change the sectionIndex (causing new problems) but will not fix the colorIndex, which is applied on Line 172. Here is a larger example that includes both the colors being wrong as well as the other parts of the chart that were implicated as I tried to fix the chart colors (border rounding, tooltips, and dynamic display using Chart.amongst):

import Browser
import Chart as C
import Chart.Attributes as CA
import Chart.Events as CE
import Chart.Item as CI
import Html as H exposing (Html)

type alias Model =
    { hovering : List (CI.One DataPoint CI.Bar) }

type Msg
    = OnHover (List (CI.One DataPoint CI.Bar))

update : Msg -> Model -> Model
update msg model =
    case msg of
        OnHover hovering ->
            { model | hovering = hovering }

main : Program () Model Msg
main =
    Browser.sandbox
        { init = init
        , update = update
        , view = viewChart
        }

type alias DataPoint =
    { x : Float
    , y : Float
    , z : Float
    }

chartData : List DataPoint
chartData =
    [ DataPoint 1 2 3
    , DataPoint 4 5 6
    , DataPoint 7 8 9
    ]

viewChart : Model -> Html Msg
viewChart model =
    C.chart
        [ CA.height 300
        , CA.width 400
        , CE.onMouseMove OnHover (CE.getNearest CI.bars)
        , CE.onMouseLeave (OnHover [])
        ]
        [ C.legendsAt .min .max [] []
        , C.bars
            [ CA.roundTop 0.1
            , CA.roundBottom 0.25
            ]
            [ C.stacked
                [ C.bar (.x >> (*) 2) [] |> C.named "2X"
                , C.bar (.y >> (*) 2) [] |> C.named "2Y"
                , C.bar (.z >> (*) 2) [] |> C.named "2Z"
                ]
                |> C.amongst model.hovering (\datum -> [ CA.highlight 0.5 ])
            , C.stacked
                [ C.bar .x [] |> C.named "X"
                , C.bar .y [] |> C.named "Y"
                , C.bar .z [] |> C.named "Z"
                ]
            ]
            chartData
        , C.barLabels [ CA.moveDown 16, CA.color "white" ]
        , C.each model.hovering <|
            \p item ->
                [ C.tooltip item [] [] [] ]
        ]

I am submitting a Pull Request that fixes the default coloring, but does not break tooltips, border rounding, or Chart.amongst (according to my research and tests).

terezka commented 7 months ago

Hi! Thanks for making me aware of this issue! It has been fix in the refactoring done for the upcoming version.