Skip to content

evaluate polished #15540

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

evaluate polished #15540

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9eb6a6c
evaluate polished
oliviertassinari Apr 30, 2019
fdc0328
try n°2
oliviertassinari Apr 30, 2019
bf07138
try es5 modules
oliviertassinari Apr 30, 2019
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
2 changes: 1 addition & 1 deletion .yarnrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
--install.frozen-lockfile true

network-timeout 150000
3 changes: 2 additions & 1 deletion packages/material-ui-lab/src/Slider/Slider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@ import React from 'react';
import ReactDOM from 'react-dom';
import PropTypes from 'prop-types';
import clsx from 'clsx';
import { fade, withStyles } from '@material-ui/core/styles';
import { withStyles } from '@material-ui/core/styles';
import { fade } from '@material-ui/core/styles/colorManipulator';
import ButtonBase from '@material-ui/core/ButtonBase';
import { setRef, withForwardedRef } from '@material-ui/core/utils';
import { clamp } from '@material-ui/lab/utils';
Expand Down
3 changes: 2 additions & 1 deletion packages/material-ui-lab/src/SpeedDialAction/SpeedDialAction.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
import React from 'react';
import PropTypes from 'prop-types';
import clsx from 'clsx';
import { emphasize, withStyles } from '@material-ui/core/styles';
import { withStyles } from '@material-ui/core/styles';
import { emphasize } from '@material-ui/core/styles/colorManipulator';
import Fab from '@material-ui/core/Fab';
import Tooltip from '@material-ui/core/Tooltip';
import { withForwardedRef } from '@material-ui/core/utils';
Expand Down
3 changes: 2 additions & 1 deletion packages/material-ui-lab/src/ToggleButton/ToggleButton.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
import React from 'react';
import PropTypes from 'prop-types';
import clsx from 'clsx';
import { fade, withStyles } from '@material-ui/core/styles';
import { withStyles } from '@material-ui/core/styles';
import { fade } from '@material-ui/core/styles/colorManipulator';
Copy link
Member

@eps1lon eps1lon May 1, 2019

Choose a reason for hiding this comment

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

@oliviertassinari This will pull in our commonJS build.

RemindMe: Write a lint rule that only allows 1st level path imports

Copy link
Member Author

@oliviertassinari oliviertassinari May 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

It deoptimizes it. Thanks!
💯 for an eslint rule. You already mentioned this problem and fixed a couple of issues in the past. But I hadn't fully realized the implications.

Copy link
Member

@eps1lon eps1lon May 1, 2019

Choose a reason for hiding this comment

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

Yeah this adds a lot of confusion. It's not obvious what imports are fine and which are not.

For v5 we should revisit our packaging strategy so that people don't even have to choose how they import from our package.

Copy link
Member Author

@oliviertassinari oliviertassinari May 1, 2019

Choose a reason for hiding this comment

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

For v5 we should revisit our packaging strategy so that people don't even have to choose how they import from our package.

What direction do you have in mind? I wish we were documenting:

import { TextField, makeStyles, Theme, PaperProps } from '@material-ui/core';

Copy link
Member

@eps1lon eps1lon May 1, 2019

Choose a reason for hiding this comment

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

Or @material-ui/TextField. Either way I think an option for the import style is helpful. We can revisit this in a separate issue. Before a change I want to give users some time to give feedback in the form of the import style they use. This change should come with a codemod.

Copy link
Member Author

@oliviertassinari oliviertassinari May 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Or @material-ui/TextField.

A past effort: #9532, it was implemented with a codemod. I can't find the other thread about it, we discussed the usage of a package per component. The @material-ui/TextField strategy has important limitations: "dependency hell" being the biggest.
I think that it's also part of the reason why people want us to implement all the component here: so they don't have dozens of dependencies to handle. It's a net plus for DX. But it also has drawbacks, it's definitely a tradeoff. Damn, I wish I can find this thread.

Copy link
Member Author

@oliviertassinari oliviertassinari May 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I have found it. I thought it was more detailed, but it seems it just me arguing against myself: #7839 (comment). To give a proper debate.

import ButtonBase from '@material-ui/core/ButtonBase';

export const styles = theme => ({
Expand Down
1 change: 1 addition & 0 deletions packages/material-ui/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
"hoist-non-react-statics": "^3.2.1",
"is-plain-object": "^2.0.4",
"normalize-scroll-left": "^0.1.2",
"polished": "^3.2.0",
"popper.js": "^1.14.1",
"prop-types": "^15.7.2",
"react-event-listener": "^0.6.6",
Expand Down
60 changes: 4 additions & 56 deletions packages/material-ui/src/styles/colorManipulator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,29 +1,6 @@
/* eslint-disable no-use-before-define */

import warning from 'warning';

/**
* Returns a number whose value is limited to the given range.
*
* @param {number} value The value to be clamped
* @param {number} min The lower boundary of the output range
* @param {number} max The upper boundary of the output range
* @returns {number} A number in the range [min, max]
*/
function clamp(value, min = 0, max = 1) {
warning(
value >= min && value <= max,
`Material-UI: the value provided ${value} is out of range [${min}, ${max}].`,
);

if (value < min) {
return min;
}
if (value > max) {
return max;
}
return value;
}
import { lighten as lighten2, darken as darken2, rgba } from 'polished';

/**
* Converts a color from CSS hex format to CSS rgb format.
Expand Down Expand Up @@ -208,15 +185,7 @@ export function emphasize(color, coefficient = 0.15) {
* @returns {string} A CSS color string. Hex input values are returned as rgb
*/
export function fade(color, value) {
color = decomposeColor(color);
value = clamp(value);

if (color.type === 'rgb' || color.type === 'hsl') {
color.type += 'a';
}
color.values[3] = value;

return recomposeColor(color);
return rgba(color, value);
}

/**
Expand All @@ -227,17 +196,7 @@ export function fade(color, value) {
* @returns {string} A CSS color string. Hex input values are returned as rgb
*/
export function darken(color, coefficient) {
color = decomposeColor(color);
coefficient = clamp(coefficient);

if (color.type.indexOf('hsl') !== -1) {
color.values[2] *= 1 - coefficient;
} else if (color.type.indexOf('rgb') !== -1) {
for (let i = 0; i < 3; i += 1) {
color.values[i] *= 1 - coefficient;
}
}
return recomposeColor(color);
return darken2(coefficient, color);
}

/**
Expand All @@ -248,16 +207,5 @@ export function darken(color, coefficient) {
* @returns {string} A CSS color string. Hex input values are returned as rgb
*/
export function lighten(color, coefficient) {
color = decomposeColor(color);
coefficient = clamp(coefficient);

if (color.type.indexOf('hsl') !== -1) {
color.values[2] += (100 - color.values[2]) * coefficient;
} else if (color.type.indexOf('rgb') !== -1) {
for (let i = 0; i < 3; i += 1) {
color.values[i] += (255 - color.values[i]) * coefficient;
}
}

return recomposeColor(color);
return lighten2(coefficient, color);
}
Loading