From 38fa4db7269948dce4e1732b561a1654a9e9c4e5 Mon Sep 17 00:00:00 2001 From: "Mehdi Rachico (mera)" Date: Tue, 5 Nov 2024 14:42:36 +0100 Subject: [PATCH] [IMP] conditional_format: improve CF data bar behavior for non-matching 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 --- src/components/translations_terms.ts | 3 - src/plugins/core/conditional_format.ts | 20 +------ .../evaluation_conditional_format.ts | 24 +++----- src/types/commands.ts | 1 - .../conditional_formatting_plugin.test.ts | 55 ++++++++++++++++++- 5 files changed, 63 insertions(+), 40 deletions(-) diff --git a/src/components/translations_terms.ts b/src/components/translations_terms.ts index ff70ae210c..a839f4fec2 100644 --- a/src/components/translations_terms.ts +++ b/src/components/translations_terms.ts @@ -27,9 +27,6 @@ export const CfTerms = { [CommandResult.ValueCellIsInvalidFormula]: _t( "At least one of the provided values is an invalid formula" ), - [CommandResult.DataBarRangeValuesMismatch]: _t( - "All the ranges and the range values must have the same size" - ), Unexpected: _t("The rule is invalid for an unknown reason"), }, ColorScale: _t("Color scale"), diff --git a/src/plugins/core/conditional_format.ts b/src/plugins/core/conditional_format.ts index 8dc4fc20c6..d4068073ec 100644 --- a/src/plugins/core/conditional_format.ts +++ b/src/plugins/core/conditional_format.ts @@ -1,5 +1,5 @@ import { compile } from "../../formulas/index"; -import { isInside, recomputeZones, toUnboundedZone, zoneToDimension } from "../../helpers/index"; +import { isInside, recomputeZones, toUnboundedZone } from "../../helpers/index"; import { AddConditionalFormatCommand, ApplyRangeChange, @@ -14,7 +14,6 @@ import { ConditionalFormatInternal, ConditionalFormattingOperatorValues, CoreCommand, - DataBarRule, ExcelWorkbookData, IconSetRule, IconThreshold, @@ -431,8 +430,6 @@ export class ConditionalFormatPlugin this.chainValidations(this.checkInflectionPoints(this.checkFormulaCompilation)) ); } - case "DataBarRule": - return this.checkDataBarRangeValues(rule, cmd.ranges, cmd.sheetId); } return CommandResult.Success; } @@ -589,21 +586,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) { diff --git a/src/plugins/ui_core_views/evaluation_conditional_format.ts b/src/plugins/ui_core_views/evaluation_conditional_format.ts index 823f85fa30..f2c7dffd3c 100644 --- a/src/plugins/ui_core_views/evaluation_conditional_format.ts +++ b/src/plugins/ui_core_views/evaluation_conditional_format.ts @@ -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 { @@ -298,8 +298,14 @@ export class EvaluationConditionalFormatPlugin extends UIPlugin { for (let row = zone.top; row <= zone.bottom; row++) { for (let col = zone.left; col <= zone.right; col++) { - const cell = this.getEvaluatedCellInZone(sheetId, zone, col, row, zoneOfValues); - if (cell.type !== CellValueType.number || cell.value <= 0) { + const targetCol = col - zone.left + zoneOfValues.left; + const targetRow = row - zone.top + zoneOfValues.top; + const cell = this.getters.getEvaluatedCell({ sheetId, col: targetCol, row: targetRow }); + if ( + !isInside(targetCol, targetRow, zoneOfValues) || + cell.type !== CellValueType.number || + cell.value <= 0 + ) { // values negatives or 0 are ignored continue; } @@ -312,18 +318,6 @@ export class EvaluationConditionalFormatPlugin extends UIPlugin { } } - private getEvaluatedCellInZone( - sheetId: UID, - zone: Zone, - col: HeaderIndex, - row: HeaderIndex, - targetZone: Zone - ) { - const targetCol = col - zone.left + targetZone.left; - const targetRow = row - zone.top + targetZone.top; - return this.getters.getEvaluatedCell({ sheetId, col: targetCol, row: targetRow }); - } - /** Compute the color scale for the given range and CF rule, and apply in in the given computedStyle object */ private applyColorScale( sheetId: UID, diff --git a/src/types/commands.ts b/src/types/commands.ts index f433daae30..b962366081 100644 --- a/src/types/commands.ts +++ b/src/types/commands.ts @@ -1311,7 +1311,6 @@ export const enum CommandResult { ValueCellIsInvalidFormula = "ValueCellIsInvalidFormula", InvalidDefinition = "InvalidDefinition", InvalidColor = "InvalidColor", - DataBarRangeValuesMismatch = "DataBarRangeValuesMismatch", } export interface CommandHandler { diff --git a/tests/conditional_formatting/conditional_formatting_plugin.test.ts b/tests/conditional_formatting/conditional_formatting_plugin.test.ts index 5c121f3226..1108540634 100644 --- a/tests/conditional_formatting/conditional_formatting_plugin.test.ts +++ b/tests/conditional_formatting/conditional_formatting_plugin.test.ts @@ -2330,7 +2330,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", @@ -2343,7 +2343,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", () => { @@ -2395,6 +2395,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 = {