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

Allow specifying gradients for DataBar conditional formatting #2296

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

erenes
Copy link

@erenes erenes commented Feb 9, 2024

Fixes #386, #1686

The dataBar element below the conditionalFormat supports a gradient property as of Excel 2010, which allows a solid fill on the conditional format.

This PR allows specifying the gradient property when creating a dataBar programatically with ClosedXML, as well as support for loading and saving files that contain solid fill dataBars.

I've tried to adhere to the existing code style and ran CodeMaid on the files in the PR. I am happy to implement unit tests as well, but did not find any existing ones for the dataBar elements.

Example code:

var workbook = new XLWorkbook();
var ws = workbook.AddWorksheet("test");
ws.FirstCell().SetValue(0.53).AddConditionalFormat()
    .DataBar(XLColor.Red, showBarOnly: false, gradient: false)
    .Minimum(XLCFContentType.Number, 0)
    .Maximum(XLCFContentType.Number, 1);
workbook.SaveAs("test.xlsx");

Copy link
Member

@jahav jahav left a comment

Choose a reason for hiding this comment

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

API has incorrect shape. It adds an optional parameter, which is not scalable for the rest of parameters ([MS-XLSX]2.6.30 CT_DataBar has many other attributes).

The Gradient prop is on IXLConditionalFormat, where it doesn't belong. It is only for DataBar (possibly other, but only subset), not generic conditional formats.

ClosedXML uses fluent API for similar case (e.g. styles are fluent something.Style.Font.SetSize(14).SetBold()).

There are two interfaces IXLCFDataBarMax and IXLCFDataBarMin. Since this thing hasn't gone through API review yet, let's just add IXLCFDataBar and chain it behind IXLCFDataBarMax (currently returns void, to ensure that there are exactly two CFVO though Min and Max interface).

interface IXLCFDataBar {
  bool Gradient {get;}
  IXLCFDataBar SetGradient(bool value = true);
}

Tests should be ClosedXml.Tests.ConditionalFormats directory. In this case, I think jut make a new ConditionalFormatDataBarTests. Test names should be Explain_what_test_does(). One test with TestHelper.CreateSaveLoadAssert should sufficie. Test file should go to ClosedXML.Tests\Resource\Other\ConditionalFormats.

Add good descriptive XML documentation for added/modified methods/props. Add user documentation for DataBar (see /doc directory). Any modification/new feature must include documentation.

Any breaking changes (in this case unification of interfaces) should be added to docs\migrations\migrate-to-0.104.rst.

@erenes
Copy link
Author

erenes commented Feb 13, 2024

Thanks for the feedback, the direction was very helpful!

I have now implemented it as a Fluent API and added a unit test.
Addtionally I've also added the breaking changes to the migrate doc, and did my best to add useful documentation (I struggled a lot to come up with anything useful for such a trivial change)

Now that I've gone this far I think it's a logical next step to get rid of the XLCFDataBarMin and XLCFDataBarMax and unify all of those calls in the new XLCFDataBar object (maybe Min can still return the Max interface, so you are nudged towards not just setting one of the two).
Let me know if you would want me to pick that up in this PR as well or to leave it as-is until there is an API review.

@erenes erenes requested a review from jahav February 13, 2024 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Losing gradient filling format
2 participants