Add new Bar and Column Series property 'ColorMode' #1272

paulo-rico · 2023-12-06T12:53:47Z

This property will enable the chart to color the bars by Series (as it is now) or by PositiveNegative, based on whether value is above or below zero.

Demo - razden.delaci.co.uk

…tive, zero or negative, respectively. Bar Chart - Position data labels right, level or left of bar if positive, zero or negative, respectively.

akorchev · 2023-12-06T14:17:35Z

Hi @paulo-rico,

Thank you for the PR. I think there a too many changes in the implementation and I can't follow all of them. Isn't it enough just to modify the PickColor method? Everything else should be the same.

paulo-rico · 2023-12-06T17:00:52Z

Hi @akorchev

It's not as straight forward. When PickColor returns null, it uses the Series position in it's collection to get the color -
var className = $"rz-column-series rz-series-{Chart.Series.IndexOf(this)}";
Whereas, PositiveNegative is based on individual values rather than series.

I'll take a look at the code and see if it can be cleaned/reduced. (I was of the same thought, initially, that only the PickColor needed adjusting).

PS - I don't know what happens to the code when pushed. In Visual Studio Git Changes panel, it shows the changes exactly as I would want to see them. In the Render methods, it showed the if (ColorMode == ColorMode.Series) as an add, and the accompanying else block as an add, and it was straight forward. When pushed and viewed on Git, it's mixing up where the add and deletes occured because of similar code. If you have any suggestions on this (other than dropping Visual Studio :) ), they would be much appreciated.

Regards

Paul

IMMEDIATE EDIT - Leave this with me. In talking it through, I think there may be a solution.

This reverts commit 0729535.

Bar and Column Render methods improved.

paulo-rico · 2023-12-06T19:12:29Z

My bad on the Render code. It was because of the initial tag <g class="@className">
I couldn't set the className unless I was inside the loop for the data. Obviously that was at the top to wrap all the render code as you can only have one root element. That I found out on the else block when I initially had to add an element to wrap it all in. I added a <div> element but it didn't render correctly. A Google search later, it seams wrapping in a <text> element is the accepted way of doing this without altering the output. But it meant that the <g> element could appear within the data loop AFTER setting the class variable.

This is why I love programming. It's a constant learning curve :)

PS I checked the changes through GitHub Desktop before committing and they appeared to be OK.

Regards

Paul

akorchev · 2023-12-07T10:07:52Z

I think we can make it better by passing the ColorMode to PickColor itself. This would reduce a lot of checks for ColorMode. Also we should probably rename the enum as ColorMode is too generic. ChartSeriesColorMode is probably better.

paulo-rico · 2023-12-07T13:12:59Z

Existing check in Render method

`
// code

            if (ChartSeriesColorMode == ChartSeriesColorMode.Series)
            {
                fill = PickColor(Items.IndexOf(data), Fills, Fill);
                stroke = PickColor(Items.IndexOf(data), Strokes, Stroke);
                className = $"rz-bar-series rz-series-{Chart.Series.IndexOf(this)}";
            }
            else
            {
                fill = PickColor(val <= 0 ? 0 : 1, Fills, Fill);
                stroke = PickColor(val <= 0 ? 0 : 1, Strokes, Stroke);
                className = val <= 0 ? $"rz-bar-series rz-series-0" : $"rz-bar-series rz-series-1";
            }

`

In moving the ChartSeriesColorMode check to PickColor, I would also have to also pass in the 'Value' of the data in order to select the appropriate color if PositiveNegative. OK so far.

But there would still be a need for the check in the Render method in order to get the className correctly based on the ChartSeriesColorMode.

Would you prefer that a new method was implemented to PickColorClass that returns the class name, passing in ChartSeriesColorMode, BaseClass (i.e. 'rz-bar-series rz-series-'), Value and Series Index? If so, would you put this in CartesianSeries alongside PickColor?

Regards

Paul

akorchev · 2023-12-07T13:44:12Z

Maybe it would be easier just to extract the index

var index = ChartSeriesColorMode == ChartSeriesColorMode.Series ? Items.IndexOf(data) : val <= 0 ? 0 : 1;

Then the rest could be:

fill = PickColor(index, Fills, Fill);
stroke = PickColor(index, Strokes, Stroke);
className = $"rz-bar-series rz-series-{index}";

paulo-rico · 2023-12-07T14:23:06Z

Almost (I do prefer the concise version!). But we need to have a separate variable for the className as it's 'Series' value is not the same as the PickColor index, i.e.

var colorIndex = ChartSeriesColorMode == ChartSeriesColorMode.Series ? Items.IndexOf(data) : val <= 0 ? 0 : 1;
var classIndex = ChartSeriesColorMode == ChartSeriesColorMode.Series ? Chart.Series.IndexOf(this) : val <= 0 ? 0 : 1;

fill = PickColor(colorIndex, Fills, Fill);
stroke = PickColor(colorIndex, Strokes, Stroke);
className = $"rz-bar-series rz-series-{classIndex}";

If you're happy with this, I'll make the changes and fire it off

EDIT: I'll fire it off anyway. Not that much of a change.

…eriesColorMode

akorchev · 2023-12-07T14:38:05Z

There seems to be a major change in the rendering implementation too. All items should be in the same <g>. Your change seems to render every item in its own <g>. This is a breaking change.

akorchev · 2023-12-07T14:39:23Z

The same refactoring should be implemented in the Color property - there is some duplication to be removed.

paulo-rico · 2023-12-07T15:08:27Z

Ok, I've changed the svg so it all wraps in a tag as per original code and applied the className to the Path object.

Not sure what Color property you mean for refactoring.
I have remove some duplicate code that sets the PositiveNegative value in the Render and moved it to a variable, i.,e.

var sign = val <= 0 ? 0 : 1;
var colorIndex = ChartSeriesColorMode == ChartSeriesColorMode.Series ? Items.IndexOf(data) : sign;
var classIndex = ChartSeriesColorMode == ChartSeriesColorMode.Series ? Chart.Series.IndexOf(this) : sign;

Is this what you meant?

akorchev · 2023-12-07T15:11:01Z

Not sure what Color property you mean for refactoring.

I mean public override string Color - it also uses similar code to find the index.

paulo-rico · 2023-12-07T16:00:28Z

Sorry @akorchev

I may need a bit of a nudge in the right direction here. All I can find for that property in various charts is -

        public override string Color
        {
            get
            {
                return Fill;
            }
        }

or something similar returning a single value with no logic. I can't find anything that suggests using an index.

akorchev · 2023-12-07T16:03:25Z

I was mislead by the diff UI. I was referring to this

radzen-blazor/Radzen.Blazor/RadzenBarSeries.razor.cs

Line 112 in 408f1dc

if (ChartSeriesColorMode == ChartSeriesColorMode.Series)

Basically search for every use of ChartSeriesColorMode. By the way the name of the property should stay ColorMode - only the enum name should be ChartSeriesColorMode.

paulo-rico · 2023-12-07T16:34:47Z

Sorry @akorchev Ignore last Push. Not changed refactored code in TooltipStyle code

akorchev · 2023-12-07T16:35:22Z

Also make sure there are no breaking changes in the rendering. The <g> element should have a class name not the <path>. I don't think you have to set the class name at all. Setting the stroke/fill will override the theme colors anyway.

paulo-rico · 2023-12-07T16:57:20Z

Just seen comment. Ignore commit. The class name needs to be set if there are no custom colors (Fills) set. When this is by series, it is OK that it set before looping through the items as it's the same for all. But with PositiveNegative, its by each data item.

What about we set the class in the <g> element as before and have a variable to hold a new className for PositiveNegative mode?

We could then interrogate that variable and either output a Path with the new class (PositiveNegative) or output a Path without any class definition, as before.

akorchev · 2023-12-07T17:44:47Z

I don't know. Rendering <g class="rz-series-8"><path class="rz-series-0" ..></path></g> feels wrong to me. The whole idea of picking the first two series for the colors of positive/negative values doesn't look correct to me. I need to think it through and probably discuss it with the team.

paulo-rico · 2023-12-07T18:22:46Z

Just going to throw some ideas out there to add to your thought process.

The thing with the PositiveNegative view is that the color selection isn't really dynamic. You must have two colors. So, do we have two special properties for this (PositiveColor?, NegativeColor?) or do we insist that there are at least two colors defined in the Fills property when ColorMode = ColorModePositiveNegative? This way, we can ignore any class manipulation and not incorporate breaking changes.

I'll give it further thought myself.

Regards and stay well

Paul

akorchev · 2023-12-07T18:24:56Z

do we insist that there are at least two colors defined in the Fills property when ColorMode = ColorModePositiveNegative?

This sounds better. I have to think a bit more.

Regards and stay well

Likewise!

paulo-rico · 2023-12-09T15:55:31Z

Inspired by this issue - #996, I think there is better, more generic way to achieve this.

Instead of dealing with PositiveNegative, we instead set a list of color ranges -

List<SeriesColorRange> colorrange = new List<SeriesColorRange>()
{
    new SeriesColorRange() {Min = -10000000, Max = 0, Color = "red"},
    new SeriesColorRange() {Min = 0, Max = 255000, Color = "orange"},
    new SeriesColorRange() {Min = 255000, Max = 10000000, Color = "#06cd06"},
};

This is a parameter on the Series

In the Render methods, we include this in the PickColor call along with Value and ColorMode -

var fill = PickColor(Items.IndexOf(data), Fills, Fill, ColorMode, FillRange, val);

Then the PickColor method becomes -

        protected string PickColor(int index, IEnumerable<string> colors, string defaultValue = null, ChartSeriesColorMode colorMode = ChartSeriesColorMode.Series, IList<SeriesColorRange> colorRange = null, double value = 0.0)
        {
            if (colorMode == ChartSeriesColorMode.Series)
            {
                if (!(colors == null || !colors.Any()))
                {
                    return colors.ElementAt(index % colors.Count());
                }
            }
            else
            {               
                if(colorRange != null)
                {
                    var result = colorRange.Where(r => r.Min <= value && r.Max > value).FirstOrDefault<SeriesColorRange>();
                    return result != null ? result.Color : defaultValue;
                }
            }

            return defaultValue;
        }

This accomodates the ability to set the color of a data item to a specific color dependant on it's value. This means all the developer has to do to achieve PositiveNegative is -

    List<SeriesColorRange> posnegrange = new List<SeriesColorRange>()
    {
        new SeriesColorRange() {Min = double.MinValue, Max = 0, Color = "red"},
        new SeriesColorRange() {Min = 0, Max = double.MaxValue, Color = "blue"},
    };

Running demo here - radzen.delaci.co.uk

This has reduced the changes to Series Render methods to adding the extra parameters to the PickColor call.

I'll commit the changes now. I don't know if you can just check this commit against the original files. I've obviously made a lot of changes back and forth.

Regards

Paul

…eNegative

akorchev · 2023-12-10T15:19:54Z

I like the idea of color ranges a lot! It seems this makes the ColorMode option obsolete doesn't it? If ColorRanges is set then use it. Otherwise don't. The default value would be null anyway. It would also make sense for other chart series at some point. What do you think?

paulo-rico · 2023-12-10T16:01:43Z

Excellent!

I can make the changes necessary to the existing Pull/Commit chain. There have been a great deal of back and forths with this. Are you able to just see the latest changes against the original file? Or would it be easier for you if I created a fresh fork and made the changes on a fresh copy (there aren't that many changes now, so it's as easy for me either way)?

I'll also make the same changes to the StackedBar and StackedColumn series. I can't see that there are any other series that would benefit from FillRange.

Paul

akorchev · 2023-12-11T09:10:14Z

I can make the changes necessary to the existing Pull/Commit chain.

I think making the changes to the current commit chain would suffice. The diff should always compare with the current master branch.

I can't see that there are any other series that would benefit from FillRange.

Probably no indeed. Only series that render a single "item" per value need this change.

… Apply to StackedSeries also.

akorchev · 2023-12-11T09:54:16Z

Radzen.Blazor/CartesianSeries.cs

            {
-                return defaultValue;
+                var result = colorRange.Where(r => r.Min <= value && r.Max > value).FirstOrDefault<SeriesColorRange>();


This should probably be r => r.Min <= value && r.Max >= value to include Max

akorchev · 2023-12-11T09:54:58Z

Radzen.Blazor/CartesianSeries.cs

+            }
+            else
+            {
+                if (!(colors == null || !colors.Any()))


I find this harder to grok than the older code in the master branch if (colors == null || !colors.Any())

akorchev · 2023-12-11T09:55:17Z

Radzen.Blazor/RadzenBarSeries.razor

@@ -24,16 +24,17 @@

        var barHeight = BandHeight;
        var height = barHeight / barSeries.Count() - padding + padding / barSeries.Count();;
-        var className = $"rz-bar-series rz-series-{Chart.Series.IndexOf(this)}";
+        var className = $"rz-column-series rz-series-{Chart.Series.IndexOf(this)}";


This should be rz-bar-series

akorchev · 2023-12-11T09:55:45Z

Radzen.Blazor/RadzenBarSeries.razor


-                <Path @key="@path" D="@path" Stroke="@stroke" StrokeWidth="@StrokeWidth" Fill="@fill" LineType="@LineType" Style="@style" />
+                <Path class="@className" @key="@path" D="@path" Stroke="@stroke" StrokeWidth="@StrokeWidth" Fill="@fill" LineType="@LineType" Style="@style" />


Should not set the class of the <path>.

akorchev · 2023-12-11T09:56:25Z

Radzen.Blazor/RadzenColumnSeries.razor

+                var fill = PickColor(Items.IndexOf(data), Fills, Fill, FillRange, val);
+                var stroke = PickColor(Items.IndexOf(data), Strokes, Stroke, StrokeRange, val);
+                
+                <Path class="@className" @key="@path" D="@path" Stroke="@stroke" StrokeWidth="@StrokeWidth" Fill="@fill" LineType="@LineType" Style="@style" />


Should not set class here as well.

akorchev · 2023-12-11T10:35:10Z

Last things (I promise)!

There seems to be trailing whitespace in some of the files (visible as big green rectangles in the diff)
Please use value instead of val (unless it isn't a conflict - if it is use seriesValue)

akorchev · 2023-12-11T10:56:44Z

Radzen.Blazor/RadzenBarSeries.razor

                var radius = Chart.BarOptions.Radius;
-
+                                


Trailing whitespace

akorchev · 2023-12-11T10:57:00Z

Radzen.Blazor/RadzenColumnSeries.razor

-
+                var fill = PickColor(Items.IndexOf(data), Fills, Fill, FillRange, itemValue);
+                var stroke = PickColor(Items.IndexOf(data), Strokes, Stroke, StrokeRange, itemValue);
+                


Trailing whitespace

akorchev · 2023-12-11T10:57:11Z

Radzen.Blazor/RadzenColumnSeries.razor.cs

@@ -26,7 +26,7 @@ public partial class RadzenColumnSeries<TItem> : CartesianSeries<TItem>, IChartC
        [Parameter]
        public IEnumerable<string> Fills { get; set; }

-        /// <summary>
+        /// <summary>   


Trailing whitespace.

paulo-rico · 2023-12-11T11:09:11Z

Hi @akorchev

A bit of help please, if you will.

In VS2022 git panel, the diffs only show the differences from my last commit, so I'm not seeing some of these things you mention. Without starting afresh, is there a way that I can view diffs for my current source against the original source as opposed to my last commit?

Cheers

Paul

akorchev · 2023-12-11T11:14:29Z

To be honest I am not sure. IDEs usually show the difference since the last commit which is expected. You would have to constantly squash your commits and compare to master if you want that. I am looking at the "Files changed" tab in Github which shows the final result.

paulo-rico · 2023-12-11T19:50:00Z

I've been looking into this, and it appears there is a way to check current source against original source.

https://devblogs.microsoft.com/visualstudio/introducing-new-git-features-to-visual-studio-2022/#compare-branches

I'll use this in future if any subsequent updates have taken a few turns/commits. Hopefully save us both some time :)

Thanks for your patience @akorchev.

Paul

akorchev · 2023-12-12T09:51:14Z

Thanks for your patience @akorchev.

Don't mention it. Thank you for your cooperation as well! Would you mind creating a demo which shows this feature? You can add it to the column chart demos.

paulo-rico · 2023-12-12T11:42:36Z

Ok, I've added it to the 'Radzen Blazor Chart column series' page in RadzenBlazorDemos project. A cursory glance at RadzenExample component suggests that the source code is read from git. That would be why it ran, but 'Edit Source' showed the existing source.

paulo-rico added 4 commits December 5, 2023 10:13

Column Chart - Position data labels above, level or below bar if posi…

7d5e944

…tive, zero or negative, respectively. Bar Chart - Position data labels right, level or left of bar if positive, zero or negative, respectively.

Merge branch 'radzenhq:master' into master

488e95a

Merge branch 'radzenhq:master' into master

ebd102e

Add new series property 'ColorMode'.

0729535

paulo-rico added 2 commits December 6, 2023 18:02

Revert "Add new series property 'ColorMode'."

173fb66

This reverts commit 0729535.

ColorMode option 'Series' or 'PositiveNegative'.

cb2fe21

Bar and Column Render methods improved.

Make Render method a bit more concise. Change property name to ChartS…

408f1dc

…eriesColorMode

Code refactoring and property name change

85d6283

Refactoring and renaming

ffa9490

Change the workings to generic ColorRanges rather than static Positiv…

acded6c

…eNegative

paulo-rico added 2 commits December 11, 2023 09:40

Remove ColorMode and redo PickColor to check for null Ranges instead.…

addd7a2

… Apply to StackedSeries also.

Removed colorMode XML Comment

ade27a6

akorchev reviewed Dec 11, 2023

View reviewed changes

Tidied up code as per comments

4a2bb87

Rename val to itemValue and remove trailing spaces from files

8ccd251

akorchev reviewed Dec 11, 2023

View reviewed changes

Radzen.Blazor/RadzenBarSeries.razor Outdated

var radius = Chart.BarOptions.Radius;

Copy link

Collaborator

akorchev Dec 11, 2023

Choose a reason for hiding this comment

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

Trailing whitespace

akorchev reviewed Dec 11, 2023

View reviewed changes

The ones I missed :)

2c4af0b

Create FillRange demo

70f97fa

akorchev merged commit 886e96f into radzenhq:master Dec 13, 2023
1 check passed


		<Path @key="@path" D="@path" Stroke="@stroke" StrokeWidth="@StrokeWidth" Fill="@fill" LineType="@LineType" Style="@style" />
		<Path class="@className" @key="@path" D="@path" Stroke="@stroke" StrokeWidth="@StrokeWidth" Fill="@fill" LineType="@LineType" Style="@style" />


		var fill = PickColor(Items.IndexOf(data), Fills, Fill, FillRange, itemValue);
		var stroke = PickColor(Items.IndexOf(data), Strokes, Stroke, StrokeRange, itemValue);

Add new Bar and Column Series property 'ColorMode' #1272

Add new Bar and Column Series property 'ColorMode' #1272

Conversation

paulo-rico commented Dec 6, 2023

akorchev commented Dec 6, 2023

paulo-rico commented Dec 6, 2023 • edited Loading

paulo-rico commented Dec 6, 2023

akorchev commented Dec 7, 2023

paulo-rico commented Dec 7, 2023 • edited Loading

akorchev commented Dec 7, 2023

paulo-rico commented Dec 7, 2023 • edited Loading

akorchev commented Dec 7, 2023

akorchev commented Dec 7, 2023

paulo-rico commented Dec 7, 2023

akorchev commented Dec 7, 2023

paulo-rico commented Dec 7, 2023

akorchev commented Dec 7, 2023

paulo-rico commented Dec 7, 2023 • edited Loading

akorchev commented Dec 7, 2023 • edited Loading

paulo-rico commented Dec 7, 2023 • edited Loading

akorchev commented Dec 7, 2023

paulo-rico commented Dec 7, 2023

akorchev commented Dec 7, 2023

paulo-rico commented Dec 9, 2023

akorchev commented Dec 10, 2023

paulo-rico commented Dec 10, 2023

akorchev commented Dec 11, 2023

akorchev Dec 11, 2023

Choose a reason for hiding this comment

akorchev Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

akorchev Dec 11, 2023

Choose a reason for hiding this comment

akorchev Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

akorchev Dec 11, 2023

Choose a reason for hiding this comment

akorchev commented Dec 11, 2023

akorchev Dec 11, 2023

Choose a reason for hiding this comment

akorchev Dec 11, 2023

Choose a reason for hiding this comment

akorchev Dec 11, 2023

Choose a reason for hiding this comment

paulo-rico commented Dec 11, 2023

akorchev commented Dec 11, 2023

paulo-rico commented Dec 11, 2023 • edited Loading

akorchev commented Dec 12, 2023

paulo-rico commented Dec 12, 2023

paulo-rico commented Dec 6, 2023 •

edited

Loading

paulo-rico commented Dec 7, 2023 •

edited

Loading

paulo-rico commented Dec 7, 2023 •

edited

Loading

paulo-rico commented Dec 7, 2023 •

edited

Loading

akorchev commented Dec 7, 2023 •

edited

Loading

paulo-rico commented Dec 7, 2023 •

edited

Loading

akorchev Dec 11, 2023 •

edited

Loading

akorchev Dec 11, 2023 •

edited

Loading

paulo-rico commented Dec 11, 2023 •

edited

Loading