[styles] Fix getLuminance for hsl #14391

strayiker · 2019-02-03T00:16:32Z

I have followed (at least) the PR section of the contributing guide.

eps1lon · 2019-02-03T08:28:55Z

You need to change the size in ./size_limit.js to fix https://circleci.com/gh/mui-org/material-ui/70262?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

oliviertassinari · 2019-02-03T09:46:15Z

Does the bundle size increase worth it? Do we have an estimation of the accuracy we win?

mbrookes · 2019-02-03T12:39:26Z

Do we have an estimation of the accuracy we win?

@strayiker gave an example in the related issue: #14387 (comment)

It can be quite significant by the looks of it, although more relevant for our purposes would be the difference in contrast ratio between a pair of colors represented as either RGB or HSL.

If we are going to make the colorManipulator API public in v4, there is another possible path forward (although @strayiker isn't going to like it 😅):

Drop support for HSL in the colorManipulator functions
Export this new function
Require conversion of HSL to RGB using this function before passing color values to Material-UI component

eps1lon · 2019-02-03T15:58:00Z

Drop support for HSL in the colorManipulator functions

Export this new function

Require conversion of HSL to RGB using this function before passing color values to Material-UI component

That seems reasonable to me. It helps with static analysis and therefore tree shaking. I don't think that it is very common to use dynamic theme colors. Themes are mostly static IMO and therefore I'd rather make use of inversion of control here. Would be nice if someone could share some use cases where color values for the theme switch dynamically between hsl and rgb.

In a perfect world we would have dynamic and static solutions and users could choose: Dynamic for experimenting, static for production.

oliviertassinari · 2019-02-05T16:49:15Z

Drop support for HSL in the colorManipulator functions

Export this new function

Require conversion of HSL to RGB using this function before passing color values to Material-UI component

@mbrookes I love this plan!
@strayiker What do you think about it?

I would like to double down on @eps1lon point. I wish we could allow our users to provide CSS variables as theme values, unfortunately, we perform some color manipulations with them. Maybe we can imagine a mode where if somebody provides a CSS variable, the color manipulation functions are identity functions?

strayiker · 2019-02-06T09:48:49Z

@mbrookes,
@oliviertassinari
What about hex format? Should it be converted to rgb manually or it will be converted internally?
I think it is better to perform convertion automatically inside the decomposeColor and drop the hsl support in the other places. I don't see any reason to put this responsibility to a developer (user of your api).
If you provide a function for converting color formats and require it to be used every time the color format does not meet the requirements, as a developer I can ask you "why can't you convert it for me"?

When the colorManupulator becomes a public, it should be format agnostic in order to be transparent in use, my opinion.

I wish we could allow our users to provide CSS variables as theme values, unfortunately, we perform some color manipulations with them. Maybe we can imagine a mode where if somebody provides a CSS variable, the color manipulation functions are identity functions?

Doesn't it lead to incorrectly looking components? If you perform some color manipulations (e.g. fade()) then using css variable will break this behaviour and, accordingly, the appearance of the components. Some states (hover, focus) can be missed.

eps1lon · 2019-02-06T10:10:06Z

I don't see any reason to put this responsibility to a developer [...]

reduced bundle size via tree shaking for developers
less maintenance burden for us (how do we decide what formats we support? How "good" should that support be? etc) Your sentiment already illustrates this: Why is full hex support ok but hsl not? Don't you think that the dev that uses hsl should have the same expectations of the API that the dev that uses hex has?

strayiker · 2019-02-06T11:05:23Z

If this difference in size is significant and worth of losing usability, then I can't argue.

mui-pr-bot · 2019-03-30T00:35:40Z

@material-ui/core: parsed: +0.17% , gzip: +0.27%
@material-ui/lab: parsed: +0.37% , gzip: +0.55%

Details of bundle changes.

Comparing: 2dedcc1...d9e99bb

bundle	parsed diff	gzip diff	prev parsed	current parsed	prev gzip	current gzip
@material-ui/core	+0.17% 🔺	+0.27% 🔺	350,243	350,825	90,024	90,263
@material-ui/core/Paper	+0.76% 🔺	+1.16% 🔺	67,867	68,380	19,823	20,053
@material-ui/core/Paper.esm	+0.91% 🔺	+1.23% 🔺	60,198	60,746	18,568	18,796
@material-ui/core/Popper	0.00%	0.00%	30,463	30,463	10,527	10,527
@material-ui/core/styles/createMuiTheme	+2.95% 🔺	+4.08% 🔺	17,384	17,897	5,729	5,963
@material-ui/core/useMediaQuery	0.00%	0.00%	2,469	2,469	1,041	1,041
@material-ui/lab	+0.37% 🔺	+0.55% 🔺	148,279	148,827	43,580	43,818
@material-ui/styles	0.00%	0.00%	53,099	53,099	15,433	15,433
@material-ui/system	0.00%	0.00%	17,136	17,136	4,525	4,525
Button	+0.62% 🔺	+0.93% 🔺	87,947	88,495	26,064	26,307
Modal	+0.67% 🔺	+0.96% 🔺	82,055	82,603	24,563	24,799
colorManipulator	+15.87% 🔺	+18.24% 🔺	3,232	3,745	1,299	1,536
docs.landing	0.00%	0.00%	49,958	49,958	10,837	10,837
docs.main	+0.08% 🔺	+0.11% 🔺	645,407	645,929	200,517	200,733
packages/material-ui/build/umd/material-ui.production.min.js	+0.17% 🔺	+0.29% 🔺	298,542	299,060	82,758	82,999

Generated by 🚫 dangerJS against d9e99bb

packages/material-ui/src/styles/colorManipulator.js

oliviertassinari · 2019-03-30T11:30:54Z

packages/material-ui/src/styles/colorManipulator.js

+  if (process.env.NODE_ENV !== 'production') {
+    if (!color) {
+      throw new Error(
+        `Material-UI: hexToRgb(color) is called with an invalid argument. (${color})`,


fast feedback for wrong usages

This adds different behavior in production. dev and prod shouldn't behave differently.

Either throw in dev too or use warning

It should be fine. It will throw in development and production. The only difference is the error message. It will be explicit in dev, obscure in prod.

Never change control flow between environments. It is not equivalent.

And even if: Why change the pattern? We always used warning. Why switch to throw here?

I think that we should answer this question first: What should happen in production when the params are invalid? We have 3 options:

We implement a defensive logic to concern the method in a no-op.

We do nothing, the logic breaks with an obscure error message.

We handle the exception, provide a nice message, with minification or not. React minification logic was implemented by an intern, it's probably overkilling at our scale.

Once we have answered this question, we can consider the development env. I would propose respectively:

We raise a warning

/ 3 We change the control flow, throw a nice warning

This problem remembers me what happen with a few of our components when none of the required props are provided. It fails with an obscure error message and a warning is print in the console:

Invariant Violation: React.cloneElement(...): The argument must be a React element, but you passed undefined.

For new react devs I agree. But a fundamental part of react debugging is the console. We can't solve any issues devs have here because react is dictating how it should work. The error overlay you cited even includes the solution to the issue:

Open your browser’s developer console to further inspect this error.

Once we have answered this question, we can consider the development env

You always start with a premise which I don't agree with and react does not agree with and I think most other people won't agree with: Thrown errors are different in prod and dev. Why should a program behave differently in production or dev (and please don't move the goalpost to warnings they're bad enough)?

This is my 2nd attempt at focusing your attention on this point. If you insist on not discussing this I'm not willing to have a discussion about the next point either.

Why should a program behave differently in production or dev

I'm gonna state it a 2nd time. The behavior is identical: it throws. Only the error message changes, like it changes with React invariant.

The react comparison is invalid. They don't use their invariants for simple types.

The behavior is identical:

Throwing is not always equivalent. This only applies for this scenario but is generally not true for e.g. side-effects.

it throws

Also not true:

hexToRgb(1); hexToRgb({ substr: () => {}})

Ok, I'm changing the approach.

eps1lon · 2019-04-03T13:09:14Z

packages/material-ui/src/styles/colorManipulator.js

-    }
+  if (!color) {
+    warning(false, `Material-UI: missing color argument in hexToRgb(${color}).`);
+    return color;


At this point it's not returning a string (unless it's an empty string) which is guaranteed by the documentation. I don't understand the use case for swallowing a TypeError here?

I'm removing all the warnings. The discussion doesn't worth it.

If people want meaningful errors, they can use TypeScript.

oliviertassinari · 2019-04-04T06:36:43Z

@strayiker Thank you.

In #14391 the Function `convertHexToRGB` was renamed to `hexToRgb`. Unfortunately, the typings were not updated to reflect the above change :( Also, an additional Function was exported, `hslToRgb`, This PR updates the `hexToRgb` typings and adds an export for `hslToRgb`.

zettca · 2019-12-23T16:31:03Z

Shouldn't this change be documented in the migration guide? It renames convertHexToRGB to hexToRgb (breaking the API), but isn't documented.

strayiker force-pushed the get-luminance branch 2 times, most recently from 364f065 to 608f339 Compare February 3, 2019 12:31

oliviertassinari added new feature New feature or request breaking change labels Feb 5, 2019

oliviertassinari changed the title ~~Fix getLuminance for hsl~~ [styles] Fix getLuminance for hsl Feb 5, 2019

oliviertassinari added the package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. label Feb 5, 2019

[styles] Fix getLuminance for hsl

49c1469

oliviertassinari changed the base branch from master to next March 30, 2019 00:30

oliviertassinari force-pushed the get-luminance branch from 608f339 to 49c1469 Compare March 30, 2019 00:30

oliviertassinari self-assigned this Mar 30, 2019

try to minimize bundle size

f7b6835

oliviertassinari reviewed Mar 30, 2019

View reviewed changes

oliviertassinari added PR: good for merge and removed breaking change labels Mar 30, 2019

oliviertassinari removed their assignment Mar 30, 2019

eps1lon removed the PR: good for merge label Mar 31, 2019

oliviertassinari self-assigned this Mar 31, 2019

sebastian review

0d41ac7

oliviertassinari added the PR: good for merge label Apr 3, 2019

oliviertassinari removed their assignment Apr 3, 2019

eps1lon reviewed Apr 3, 2019

View reviewed changes

remove the warnings

d9e99bb

oliviertassinari merged commit 2578cb4 into mui:next Apr 4, 2019

whitneyit mentioned this pull request Jun 5, 2019

[core] Fix incorrect typings for hexToRgb #16059

Merged

1 task

eps1lon added the breaking change label Dec 27, 2019

eps1lon mentioned this pull request Dec 27, 2019

Document convertHexToRGB migration #18995

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[styles] Fix getLuminance for hsl #14391

[styles] Fix getLuminance for hsl #14391

strayiker commented Feb 3, 2019

eps1lon commented Feb 3, 2019

oliviertassinari commented Feb 3, 2019

mbrookes commented Feb 3, 2019 •

edited

Loading

eps1lon commented Feb 3, 2019

oliviertassinari commented Feb 5, 2019 •

edited

Loading

strayiker commented Feb 6, 2019

eps1lon commented Feb 6, 2019

strayiker commented Feb 6, 2019

mui-pr-bot commented Mar 30, 2019 •

edited

Loading

oliviertassinari Mar 30, 2019

eps1lon Mar 31, 2019

oliviertassinari Mar 31, 2019 •

edited

Loading

eps1lon Mar 31, 2019

eps1lon Mar 31, 2019

oliviertassinari Mar 31, 2019 •

edited

Loading

eps1lon Mar 31, 2019

oliviertassinari Mar 31, 2019 •

edited

Loading

eps1lon Mar 31, 2019

oliviertassinari Apr 1, 2019

eps1lon Apr 3, 2019

oliviertassinari Apr 3, 2019

oliviertassinari Apr 3, 2019

oliviertassinari commented Apr 4, 2019

zettca commented Dec 23, 2019

[styles] Fix getLuminance for hsl #14391

[styles] Fix getLuminance for hsl #14391

Conversation

strayiker commented Feb 3, 2019

eps1lon commented Feb 3, 2019

oliviertassinari commented Feb 3, 2019

mbrookes commented Feb 3, 2019 • edited Loading

eps1lon commented Feb 3, 2019

oliviertassinari commented Feb 5, 2019 • edited Loading

strayiker commented Feb 6, 2019

eps1lon commented Feb 6, 2019

strayiker commented Feb 6, 2019

mui-pr-bot commented Mar 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Mar 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Mar 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Mar 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented Apr 4, 2019

zettca commented Dec 23, 2019

mbrookes commented Feb 3, 2019 •

edited

Loading

oliviertassinari commented Feb 5, 2019 •

edited

Loading

mui-pr-bot commented Mar 30, 2019 •

edited

Loading

oliviertassinari Mar 31, 2019 •

edited

Loading

oliviertassinari Mar 31, 2019 •

edited

Loading

oliviertassinari Mar 31, 2019 •

edited

Loading