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

Second revision of pivot styles API #1186 #1222

Open
wants to merge 5 commits into
base: develop
from

Conversation

Projects
None yet
3 participants
@b0bi79
Copy link
Member

commented May 29, 2019

What's this PR do?

The refined API now allows to assign styles for:

  • each individual value column;
  • the totals of each individual value column;
  • labels of the totals;
  • any kind of totals, not just for DefaultSubtotal.
  • correctly load styles of files created in MS Excel

Contains fix for #1186
Cancels #1189
Replaces #1129

b0bi79 added some commits May 29, 2019

@b0bi79

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

This PR is needed for the release of ClosedXML.Report

@igitur igitur added this to the v0.95 milestone Jun 5, 2019

@igitur igitur self-requested a review Jun 5, 2019

@igitur igitur added the enhancement label Jun 5, 2019

@Pankraty Pankraty added the .Report label Jun 5, 2019

@igitur

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

I'm finally looking at this PR. But it seems like you have undone many of the changes I made when we last worked on the pivot table formats. For example, now we're back to using public interface IFieldRef again, which is not a good public name for an interface. Also, some of the old patterns are back too.

@b0bi79

This comment has been minimized.

Copy link
Member Author

commented Jul 6, 2019

I had to go back to the old models because the models you entered could not describe all the options for using Pivot styles. And for this reason, I had to make many interfaces open.

@b0bi79

This comment has been minimized.

Copy link
Member Author

commented Jul 6, 2019

Sorry, Francois. I again did ugly interface. If I understood what you don't like, maybe I could have done better. And it seemed to me that we agreed with you that I would make a public interface like yours, and an additional "low-level" API with lambda functions, to which you agreed.

var brightYellow = XLColor.FromArgb(255, 255, 153);
var gray = XLColor.FromArgb(192, 192, 192);

companyField.AddHeaderStyle().Style.Font.SetBold();

This comment has been minimized.

Copy link
@igitur

igitur Jul 8, 2019

Member

I don't see any fields in the output file that display as bold.

This comment has been minimized.

Copy link
@b0bi79

b0bi79 Jul 8, 2019

Author Member

Fixed

@igitur

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

Is it possible that you can generate a pivot table that illustrates every kind of pivot table format? If I understand the limitations of the current implementation on the develop branch, then I will understand better what this PR intends.

@b0bi79

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2019

At first it seemed to me that the options that you described should describe everything possible. But when I began to open and save different documents with pivot tables, I realized that it was impossible to describe everything (or it was very difficult). Therefore, I decided that it is better to return to the API that you do not like.

Show resolved Hide resolved ClosedXML/Excel/PivotTables/PivotStyleFormats/XLPivotStyleFormatEnums.cs

public enum XLPivotTableAxisValues
{
AxisRow = 0,

This comment has been minimized.

Copy link
@Pankraty

Pankraty Jul 9, 2019

Member

Same here. Unless the particular numbers have some meaning (i.e. they coincide to OpenXML values), it is better to reserve 0 to NotSet than use an arbitrary value as a default for non-assigned variables. It protects from rare unpleasant bugs.

{ }
public XLPivotValueStyleFormat(IXLPivotFormat pivotFormat)
{
PivotFormat = pivotFormat;

This comment has been minimized.

Copy link
@Pankraty

Pankraty Jul 9, 2019

Member

Better check the input for null here than have NRE in Style getter.

public IXLPivotValueStyleFormat AndWith(IXLPivotField field)
{
return AndWith(field, null);
}

public IXLPivotValueStyleFormat AndWith(IXLPivotField field, Predicate<Object> predicate)
{
FieldReferences.Add(new PivotLabelFieldReference(field, predicate));
((List<IFieldRef>)PivotFormat.FieldReferences).Add(FieldRef.ForField(field.SourceName, predicate));

This comment has been minimized.

Copy link
@Pankraty

Pankraty Jul 9, 2019

Member

Better avoid typecast here. For example, in the constructor, you can accept a concrete implementation of IXLPivotFormat that allows modification of FieldReferences collection. Or define an internal interface for that. I can provide a more detailed explanation if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.