Skip to content
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

Jump to bottom

[ButtonBase] Remove dead style #15503

Merged
merged 5 commits into from
Apr 27, 2019
Merged

[ButtonBase] Remove dead style #15503

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7683812
use JSS for ripple effect
Apr 26, 2019
0c3aa95
fix formatting error
Apr 26, 2019
3d83679
use single function to pass props into styles
Apr 26, 2019
7b9e4e3
alter TouchRipple to avoid needing index
Apr 26, 2019
b572e0c
revert Ripple changes
oliviertassinari Apr 27, 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
21 changes: 13 additions & 8 deletions packages/material-ui/src/ButtonBase/Ripple.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,24 @@
import React from 'react';
import PropTypes from 'prop-types';
import clsx from 'clsx';
import { makeStyles } from '@material-ui/styles';
import { Transition } from 'react-transition-group';

const useStyles = makeStyles({
root: props => ({
width: props.rippleSize,
height: props.rippleSize,
top: -(props.rippleSize / 2) + props.rippleY,
left: -(props.rippleSize / 2) + props.rippleX,
}),
});

/**
* @ignore - internal component.
*/
function Ripple(props) {
const { classes, className, pulsate, rippleX, rippleY, rippleSize, ...other } = props;
const rippleClasses = useStyles({ rippleX, rippleY, rippleSize });
const [visible, setVisible] = React.useState(false);
const [leaving, setLeaving] = React.useState(false);

Expand All @@ -26,23 +37,17 @@ function Ripple(props) {
[classes.ripplePulsate]: pulsate,
},
className,
rippleClasses.root,
);

const rippleStyles = {
Copy link
Member

@oliviertassinari oliviertassinari Apr 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Could you revert this change? I believe that removing the !important style that overrides the inline style should be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you suggesting we keep the inline styles in addition to the new class?

Copy link
Member

@oliviertassinari oliviertassinari Apr 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Do you confirm that we only need #15503 (comment) to solve the problem?

Copy link
Member

@oliviertassinari oliviertassinari Apr 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I would rather use inline style than the style function (in our case).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I see what you are saying. No, that resolves the need to use the index option on makeStyles, but it would not resolve my issue without the rest of the changes. I have to run my code on websites with crazy garbage like span { width: 200px !Important; } set in their stylesheets. So I have to clean that up in my namespace with a reset including things like .myNamespace * { width: auto !important; }.

What is the aversion to moving to a style function, performance? Just seems like it would be nice to solve the CSP issue and allow any sort of preprocessing to run on these styles that users would like.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the extra detail, I better understand the problem now. There is close to no performance implication in this case, but it will once it's made a systematic solution. We might be able to use CSS variables in v5 (#14420). I have reverted the change of this file.
Did you consider using the shadow DOM?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would love to be using shadow DOM, it is perfect for my use case. I may implement it for most users again soon (past attempted this in MUI 0.x and ran into too many issues).

That said, unfortunately we have a ton of IE11 and Edge users. The polyfill basically does what I am doing with my jss plugins, namespaces all of the CSS. So in those browsers I will still have this issue.

I understand not wanting to pull in this change, I will probably just patch it locally for now. Would you mind giving some insight into where you think I will hit a performance issue when changing the other cases of inline style usage?

Copy link
Member

@oliviertassinari oliviertassinari Apr 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I can imagine that supporting IE 11 is important for you, shadow DOM is not an option. Dropping IE 11 would also enable CSS variables and probably solve the issue here at the same time.

I'm curious why is dynamically creating an iframe is not an option for you? We do it for a few of our demos in the documentation, e.g. the Drawers' demos. If you are familiar with Stripe, it's how they increase the security of their web input widget.

Using style functions is slower. If it's only 10% slower (end-to-end), it worth it, 💯 ! It would unlock a lot of interesting features for Material-UI. I'm happy to push this further but in a different pull request. Removing the dead CSS rules is already an improvement :).

Copy link
Member

Choose a reason for hiding this comment

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

This discussion echos with the one of #15499 as it requires the usage of style functions in the core components.

width: rippleSize,
height: rippleSize,
top: -(rippleSize / 2) + rippleY,
left: -(rippleSize / 2) + rippleX,
};

const childClassName = clsx(classes.child, {
[classes.childLeaving]: leaving,
[classes.childPulsate]: pulsate,
});

return (
<Transition onEnter={handleEnter} onExit={handleExit} {...other}>
<span className={rippleClassName} style={rippleStyles}>
<span className={rippleClassName}>
<span className={childClassName} />
</span>
</Transition>
Expand Down
4 changes: 0 additions & 4 deletions packages/material-ui/src/ButtonBase/TouchRipple.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,6 @@ export const styles = theme => ({
},
/* Styles applied to the internal `Ripple` components `ripple` class. */
ripple: {
width: 50,
height: 50,
left: 0,
top: 0,
opacity: 0,
position: 'absolute',
},
Expand Down