Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Remove strict param, rename variables, use epsilons
  • Loading branch information
jsnajdr committed Nov 7, 2025
commit 044f654702ebff8eb24f9aee2662736a8feca538
38 changes: 13 additions & 25 deletions packages/theme/src/color-ramps/lib/find-color-with-constraints.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import { type TaperChromaOptions, taperChroma } from './taper-chroma';
* @param target
* @param direction
* @param options
* @param options.strict
* @param options.lightnessConstraint
* @param options.lightnessConstraint.type
* @param options.lightnessConstraint.value
Expand All @@ -42,14 +41,12 @@ export function findColorMeetingRequirements(
{
lightnessConstraint,
taperChromaOptions,
strict = true,
Copy link
Member Author

Choose a reason for hiding this comment

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

I removed the strict param because it was never used. If true, it caused an exception to be thrown if a matching color can't be found. But we don't need that, we return a lot of information about why a match wasn't found: achieved, reached, deficit, ...

}: {
lightnessConstraint?: {
type: 'force' | 'onlyIfSucceeds';
value: number;
};
taperChromaOptions?: TaperChromaOptions;
strict?: boolean;
} = {}
): { color: ColorTypes; reached: boolean; achieved: number; deficit?: number } {
// A target of 1 means same color.
Expand Down Expand Up @@ -86,6 +83,11 @@ export function findColorMeetingRequirements(

const contrastWithSeed = getContrast( reference, seed );

// Set the boundary based on the direction.
const mostContrastingL = direction === 'lighter' ? 1 : 0;
const mostContrastingColor = direction === 'lighter' ? WHITE : BLACK;
const highestContrast = getContrast( reference, mostContrastingColor );
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved this block few lines up because we use the highestContrast value to calculate the deficit return field.


if ( lightnessConstraint ) {
// Apply a specific L value.
// Useful when pinning a step to a specific lightness, of to specify
Expand All @@ -101,36 +103,22 @@ export function findColorMeetingRequirements(
) {
return {
color: colorWithExactL,
reached: exactLContrast >= target,
reached: exactLContrast + CONTRAST_EPSILON >= target,
achieved: exactLContrast,
deficit:
exactLContrast >= target
? undefined
: ( target - exactLContrast ) * contrastWithSeed,
( exactLContrast >= target
? exactLContrast - highestContrast
: target - exactLContrast ) * contrastWithSeed,
};
}
}

// Set the boundary based on the direction.
const mostContrastingL = direction === 'lighter' ? 1 : 0;
const mostContrastingColor = direction === 'lighter' ? WHITE : BLACK;
const highestContrast = getContrast( reference, mostContrastingColor );

// If even the most contrasting color can't reach the target,
// the target is unreachable.
if ( highestContrast < target ) {
if ( strict ) {
throw new Error(
`Contrast target ${ target.toFixed(
2
) }:1 unreachable in ${ direction } direction` +
`(boundary achieves ${ highestContrast.toFixed( 3 ) }:1).`
);
}

// If even the most contrasting color can't reach the target, the target is unreachable.
// On the othe hand, if the contrast is very close to the target, we consider it reached.
if ( highestContrast < target + CONTRAST_EPSILON ) {
return {
color: mostContrastingColor,
reached: false,
reached: highestContrast + CONTRAST_EPSILON >= target,
achieved: highestContrast,
deficit: ( target - highestContrast ) * contrastWithSeed,
};
Expand Down
16 changes: 9 additions & 7 deletions packages/theme/src/color-ramps/lib/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,9 @@ function calculateRamp( {
keyof Ramp,
{ color: string; warning: boolean }
>;
let DEFICIT_DIRECTION: RampDirection = 'lighter';
let MAX_DEFICIT = -Infinity;
let MAX_DEFICIT_DIRECTION: RampDirection = 'lighter';
let MAX_DEFICIT_STEP = 'none';

// Keep track of the calculated colors, as they are going to be useful
// when other colors reference them.
Expand Down Expand Up @@ -165,7 +166,6 @@ function calculateRamp( {
adjustedTarget,
computedDir,
{
strict: false,
lightnessConstraint,
taperChromaOptions,
}
Expand All @@ -179,7 +179,8 @@ function calculateRamp( {
searchResults.deficit > MAX_DEFICIT
) {
MAX_DEFICIT = searchResults.deficit;
DEFICIT_DIRECTION = computedDir;
MAX_DEFICIT_DIRECTION = computedDir;
MAX_DEFICIT_STEP = stepName;
}
}

Expand All @@ -195,8 +196,9 @@ function calculateRamp( {
}
return {
rampResults,
DEFICIT_DIRECTION,
MAX_DEFICIT,
MAX_DEFICIT_DIRECTION,
MAX_DEFICIT_STEP,
};
}

Expand Down Expand Up @@ -243,7 +245,7 @@ export function buildRamp(
const sortedSteps = sortByDependency( config );

// Calculate the ramp with the initial seed.
const { rampResults, DEFICIT_DIRECTION, MAX_DEFICIT } = calculateRamp( {
const { rampResults, MAX_DEFICIT, MAX_DEFICIT_DIRECTION } = calculateRamp( {
seed,
sortedSteps,
config,
Expand All @@ -264,7 +266,7 @@ export function buildRamp(

// For a scale with the "lighter" direction, the contrast can be improved
// by darkening the seed. For "darker" direction, by lightening the seed.
let betterSeedL = DEFICIT_DIRECTION === 'lighter' ? 0 : 1;
let betterSeedL = MAX_DEFICIT_DIRECTION === 'lighter' ? 0 : 1;
let betterDeficit = -MAX_DEFICIT;
let betterReplaced = false;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the same "Illinois bisection" as used in findColorMeetingRequirements. If we continue to use it (I'm not sure about it) I'll abstract it into a common function.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about it

What are your reservations about it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Today I implemented another optimization that, when adjusting the seed, recomputes only the few "critical" colors instead of the entire ramp. Yesterday, when it was still only in my head, I wasn't yet sure if there will be really two bisection loops that are similar to each other. It turns out there are, and I no longer have the reservations I had.

Before merging this PR, I'll extract the "solve by bisection" function to an util and also address the other minor feedback.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is now a solveWithBisect helper that's used at two places.


Expand Down Expand Up @@ -296,7 +298,7 @@ export function buildRamp(
// Don't use the `MAX_DEFICIT` value because it's not related to our search,
// and might even be positive, which would confuse the bisection algorithm.
bestDeficit =
iterationResults.DEFICIT_DIRECTION === DEFICIT_DIRECTION
iterationResults.MAX_DEFICIT_DIRECTION === MAX_DEFICIT_DIRECTION
? iterationResults.MAX_DEFICIT
: -MAX_DEFICIT;

Expand Down