Skip to content

Commit

Permalink
[IMP] conditional_format: improve CF data bar behavior for non-matchi…
Browse files Browse the repository at this point in the history
…ng range sizes

Before this commit, defining a CF data bar rule based on the value of
another range requires both ranges to have matching sizes, preventing
the CF from being created otherwise. This restriction leads to a poor
user experience when copying and pasting CFs, as the CF does not get
pasted.

This commit removes the restriction, allowing CF creation in a "best effort"
manner when the range sizes do not match. The CF style is therefore applied by
comparing the matching cells.

Task: 4280720
  • Loading branch information
Rachico committed Nov 12, 2024
1 parent 589eee0 commit 5fc35be
Show file tree
Hide file tree
Showing 3 changed files with 60 additions and 28 deletions.
26 changes: 1 addition & 25 deletions src/plugins/core/conditional_format.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,5 @@
import { compile } from "../../formulas/index";
import {
deepEquals,
isInside,
recomputeZones,
toUnboundedZone,
zoneToDimension,
} from "../../helpers/index";
import { deepEquals, isInside, recomputeZones, toUnboundedZone } from "../../helpers/index";
import {
AddConditionalFormatCommand,
ApplyRangeChange,
Expand All @@ -20,7 +14,6 @@ import {
ConditionalFormatInternal,
ConditionalFormattingOperatorValues,
CoreCommand,
DataBarRule,
ExcelWorkbookData,
IconSetRule,
IconThreshold,
Expand Down Expand Up @@ -442,8 +435,6 @@ export class ConditionalFormatPlugin
this.chainValidations(this.checkInflectionPoints(this.checkFormulaCompilation))
);
}
case "DataBarRule":
return this.checkDataBarRangeValues(rule, cmd.ranges, cmd.sheetId);
}
return CommandResult.Success;
}
Expand Down Expand Up @@ -614,21 +605,6 @@ export class ConditionalFormatPlugin
return CommandResult.Success;
}

private checkDataBarRangeValues(rule: DataBarRule, ranges: RangeData[], sheetId: UID) {
if (rule.rangeValues) {
const { numberOfCols, numberOfRows } = zoneToDimension(
this.getters.getRangeFromSheetXC(sheetId, rule.rangeValues).zone
);
for (const range of ranges) {
const dimensions = zoneToDimension(this.getters.getRangeFromRangeData(range).zone);
if (numberOfCols !== dimensions.numberOfCols || numberOfRows !== dimensions.numberOfRows) {
return CommandResult.DataBarRangeValuesMismatch;
}
}
}
return CommandResult.Success;
}

private removeConditionalFormatting(id: string, sheet: string) {
const cfIndex = this.cfRules[sheet].findIndex((s) => s.id === id);
if (cfIndex !== -1) {
Expand Down
7 changes: 6 additions & 1 deletion src/plugins/ui_core_views/evaluation_conditional_format.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { compile } from "../../formulas";
import { parseLiteral } from "../../helpers/cells";
import { colorNumberString, percentile } from "../../helpers/index";
import { colorNumberString, isInside, percentile } from "../../helpers/index";
import { clip, largeMax, largeMin, lazy } from "../../helpers/misc";
import { _t } from "../../translation";
import {
Expand Down Expand Up @@ -320,6 +320,11 @@ export class EvaluationConditionalFormatPlugin extends UIPlugin {
) {
const targetCol = col - zone.left + targetZone.left;
const targetRow = row - zone.top + targetZone.top;
// if the target cell is outside the CF rangeValues
// we return -1 to avoid applying the data bar rule
if (!isInside(targetCol, targetRow, targetZone)) {
return { type: CellValueType.number, value: -1 };
}
return this.getters.getEvaluatedCell({ sheetId, col: targetCol, row: targetRow });
}

Expand Down
55 changes: 53 additions & 2 deletions tests/conditional_formatting/conditional_formatting_plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2345,7 +2345,7 @@ describe("conditional formats types", () => {
expect(getStyle(model, "A1")).toEqual({});
});

test("DataBar command is refused if the rangeValue cell has not the same size", () => {
test("Can add a data bar rule with rangeValue having a differnet size than the cf range", () => {
const result = model.dispatch("ADD_CONDITIONAL_FORMAT", {
cf: {
id: "1",
Expand All @@ -2358,7 +2358,7 @@ describe("conditional formats types", () => {
ranges: toRangesData(sheetId, "A1"),
sheetId,
});
expect(result).toBeCancelledBecause(CommandResult.DataBarRangeValuesMismatch);
expect(result).toBeSuccessfullyDispatched;
});

test("Can add a data bar cf based on itself", () => {
Expand Down Expand Up @@ -2410,6 +2410,57 @@ describe("conditional formats types", () => {
expect(getDataBarFill(model, "B3")?.percentage).toBe(100);
});

test("Adding a data bar rule with rangeValues having smaller size than cf range should only apply rule on matching cells", () => {
// prettier-ignore
const grid = {
A1: "A", B1: "2",
A2: "B", B2: "4",
A3: "C", B3: "8",
};
const model = createModelFromGrid(grid);
model.dispatch("ADD_CONDITIONAL_FORMAT", {
cf: {
id: "1",
rule: {
type: "DataBarRule",
color: 0xff0000,
rangeValues: "B1:B2",
},
},
ranges: toRangesData(sheetId, "A1:A3"),
sheetId,
});
expect(getDataBarFill(model, "A1")?.percentage).toBe(50);
expect(getDataBarFill(model, "A2")?.percentage).toBe(100);
expect(getDataBarFill(model, "A3")?.percentage).toBeUndefined();
});

test("Adding a data bar rule with rangeValues having bigger size than cf range should only apply rule on matching cells", () => {
// prettier-ignore
const grid = {
A1: "A", B1: "2",
A2: "B", B2: "4",
A3: "C", B3: "8",
};
const model = createModelFromGrid(grid);
model.dispatch("ADD_CONDITIONAL_FORMAT", {
cf: {
id: "1",
rule: {
type: "DataBarRule",
color: 0xff0000,
rangeValues: "B1:B4",
},
},
ranges: toRangesData(sheetId, "A1:A3"),
sheetId,
});
expect(getDataBarFill(model, "A1")?.percentage).toBe(25);
expect(getDataBarFill(model, "A2")?.percentage).toBe(50);
expect(getDataBarFill(model, "A3")?.percentage).toBe(100);
expect(getDataBarFill(model, "A4")?.percentage).toBeUndefined();
});

test("Data bar CF with negative values", () => {
// prettier-ignore
const grid = {
Expand Down

0 comments on commit 5fc35be

Please sign in to comment.