Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
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
1 change: 1 addition & 0 deletions UNRELEASED.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ Use [the changelog guidelines](https://git.io/polaris-changelog-guidelines) to f
- Fixed `Filters` duplicated `ConnectedFilter` ids ([#3651](https://github.com/Shopify/polaris-react/pull/3651))
- Fixed `Banner` `secondaryAction` only rendering if `action` is set ([#2949](https://github.com/Shopify/polaris-react/pull/2949))
- Added a `alwaysRenderCustomProperties` to `ThemeProvider` for elements that render outside of the DOM tree to their parent context ([#3652](https://github.com/Shopify/polaris-react/pull/3652))
- Increased precision of hue, saturation, lightness, and alpha in HSBLA `color-transformers` (https://github.com/Shopify/polaris-react/pull/3640)

### Documentation

Expand Down
4 changes: 2 additions & 2 deletions src/components/ThemeProvider/tests/ThemeProvider.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ describe('<ThemeProvider />', () => {
expect(themeProvider.find('div')).toHaveReactProps({
style: expect.objectContaining({
'--top-bar-background': '#108043',
'--top-bar-background-lighter': 'hsla(147, 63%, 43%, 1)',
'--top-bar-background-lighter': 'hsla(147.32, 62.78%, 43.24%, 1)',
'--top-bar-color': 'rgb(255, 255, 255)',
'--top-bar-border': 'rgb(196, 205, 213)',
}),
Expand Down Expand Up @@ -127,7 +127,7 @@ describe('<ThemeProvider />', () => {
expect(themeProvider.find('div')).toHaveReactProps({
style: expect.objectContaining({
'--top-bar-background': '#021123',
'--top-bar-background-lighter': 'hsla(213, 74%, 22%, 1)',
'--top-bar-background-lighter': 'hsla(212.73, 74.19%, 22.25%, 1)',
'--top-bar-color': 'rgb(255, 255, 255)',
'--top-bar-border': 'rgb(196, 205, 213)',
}),
Expand Down
39 changes: 26 additions & 13 deletions src/utilities/color-transformers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import type {
HSLAColor,
HSBLAColor,
} from './color-types';
import {roundNumberToDecimalPlaces} from './roundNumberToDecimalPlaces';

export function rgbString(color: RGBColor | RGBAColor) {
const {red, green, blue} = color;
Expand Down Expand Up @@ -165,14 +166,15 @@ function rgbToHsbl(color: RGBAColor, type: 'b' | 'l' = 'b'): HSBLAColor {
huePercentage = (red - green) / delta + 4;
}

const hue = Math.round((huePercentage / 6) * 360);
const hue = (huePercentage / 6) * 360;
const clampedHue = clamp(hue, 0, 360);

return {
hue: clamp(hue, 0, 360) || 0,
saturation: parseFloat(clamp(saturation, 0, 1).toFixed(2)),
brightness: parseFloat(clamp(largestComponent, 0, 1).toFixed(4)),
lightness: parseFloat(lightness.toFixed(2)),
alpha: parseFloat(alpha.toFixed(2)),
hue: clampedHue ? roundNumberToDecimalPlaces(clampedHue, 2) : 0,
saturation: roundNumberToDecimalPlaces(clamp(saturation, 0, 1), 4),
brightness: roundNumberToDecimalPlaces(clamp(largestComponent, 0, 1), 4),
lightness: roundNumberToDecimalPlaces(lightness, 4),
alpha: roundNumberToDecimalPlaces(alpha, 4),
};
}

Expand All @@ -190,8 +192,10 @@ export function rgbToHsl(color: RGBAColor): HSLAColor {
lightness: rawLightness,
alpha = 1,
} = rgbToHsbl(color, 'l');
const saturation = rawSaturation * 100;
const lightness = rawLightness * 100;

const saturation = roundNumberToDecimalPlaces(rawSaturation * 100, 2);
const lightness = roundNumberToDecimalPlaces(rawLightness * 100, 2);

return {hue, saturation, lightness, alpha};
}

Expand Down Expand Up @@ -242,7 +246,16 @@ export function hslToString(hslColor: HSLColor | HSLAColor | string) {

const alpha = 'alpha' in hslColor ? hslColor.alpha : 1;
const {hue, lightness, saturation} = hslColor;
return `hsla(${hue}, ${saturation}%, ${lightness}%, ${alpha})`;
return `hsla(${roundNumberToDecimalPlaces(
hue,
2,
)}, ${roundNumberToDecimalPlaces(
saturation,
2,
)}%, ${roundNumberToDecimalPlaces(
lightness,
2,
)}%, ${roundNumberToDecimalPlaces(alpha, 2)})`;
}

function rgbToObject(color: string): RGBAColor {
Expand Down Expand Up @@ -281,10 +294,10 @@ function hslToObject(color: string): HSLAColor {

const [hue, saturation, lightness, alpha] = colorMatch[1].split(',');
const objColor = {
hue: parseInt(hue, 10),
saturation: parseInt(saturation, 10),
lightness: parseInt(lightness, 10),
alpha: parseFloat(alpha) || 1,
hue: roundNumberToDecimalPlaces(parseFloat(hue), 2),
saturation: roundNumberToDecimalPlaces(parseFloat(saturation), 2),
lightness: roundNumberToDecimalPlaces(parseFloat(lightness), 2),
alpha: roundNumberToDecimalPlaces(parseFloat(alpha), 2) || 1,
};
return objColor;
}
Expand Down
13 changes: 13 additions & 0 deletions src/utilities/roundNumberToDecimalPlaces.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Because everything is a float in JS, Number.toFixed sometimes rounds in the
// "wrong" direction because of float imprecision. For instance:
// `(1.005).toFixed(2)` is `1.00`, NOT `1.01` because 1.005 in floating point is
// actually 1.004999995. By using exponentiation tricks here we can work around
// this imprecision, so `roundNumberToDecimalPlaces(1.005)` returns the expected
// value of `1.01`
// See https://www.jacklmoore.com/notes/rounding-in-javascript/
export function roundNumberToDecimalPlaces(value: number, decimals: number) {
const exponent = Number(`${value}e${decimals}`);
const roundedExponent = Math.round(exponent);
const numberWithDecimalPlaces = Number(`${roundedExponent}e-${decimals}`);
return numberWithDecimalPlaces;
}
Copy link
Contributor Author

@MLDimitry MLDimitry Nov 16, 2020

Choose a reason for hiding this comment

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

On my continued exploration of floating point fun, I found that toFixed(2) (which uses Math.round for it's rounding) does not accurately round all floating point numbers. i.e. 1.005 in floating point is really 1.004999995, or a value very close but below 1.005 which causes it to be rounded to 1.00 instead of 1.01. (I have a test for this case)

Here's an interesting blog post on rounding which led me to this solution which converts the numbers to exponential notation for more accurate rounding.

27 changes: 20 additions & 7 deletions src/utilities/tests/color-transformers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -131,24 +131,37 @@ describe('colorUtilities', () => {
expect(saturation).toBe(0);
expect(brightness).toBe(0.5294);
});

// test for an issue where Hex colours were losing precision, due to hue rounding
// https://github.com/Shopify/shopify/issues/265949
it('returns 16.5517/0.521/0.6549 hsb value when passed rgb(167, 104, 80)', () => {
const {hue, saturation, brightness} = rgbToHsb({
red: 167,
green: 104,
blue: 80,
});
expect(hue).toBe(16.55);
expect(saturation).toBe(0.521);
expect(brightness).toBe(0.6549);
});
});

describe('colorToHsla', () => {
it('returns the hsla color for hex', () => {
expect(colorToHsla('#dddddd')).toStrictEqual({
alpha: 1,
hue: 0,
lightness: 87,
lightness: 86.67,
saturation: 0,
});
});

it('returns the hsla color for rgb', () => {
expect(colorToHsla('rgb(132, 11, 2)')).toStrictEqual({
alpha: 1,
hue: 4,
lightness: 26,
saturation: 97,
hue: 4.15,
lightness: 26.27,
saturation: 97.01,
});
});

Expand All @@ -164,9 +177,9 @@ describe('colorUtilities', () => {
it('returns the hsla color for rgba', () => {
expect(colorToHsla('rgb(132, 11, 2, 0.2)')).toStrictEqual({
alpha: 1,
hue: 4,
lightness: 26,
saturation: 97,
hue: 4.15,
lightness: 26.27,
saturation: 97.01,
});
});

Expand Down
10 changes: 10 additions & 0 deletions src/utilities/tests/roundNumberToDecimals.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import {roundNumberToDecimalPlaces} from '../roundNumberToDecimalPlaces';

describe('roundNumberToDecimalPlaces', () => {
it('rounds 1.004 to 1.00', () => {
expect(roundNumberToDecimalPlaces(1.004, 2)).toBe(1.0);
});
it('rounds 1.005 to 1.01', () => {
expect(roundNumberToDecimalPlaces(1.005, 2)).toBe(1.01);
});
});