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.

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

[Toggle] Remove style-propable mixin #3189

Merged
merged 1 commit into from
Feb 5, 2016

Conversation

newoga
Copy link
Contributor

@newoga newoga commented Feb 5, 2016

No description provided.

@oliviertassinari
Copy link
Member

@newoga That looks good 👍.

@newoga
Copy link
Contributor Author

newoga commented Feb 5, 2016

@oliviertassinari I'm done with this, should we merge, get another pair of eyes to take a look?

@alitaheri
Copy link
Member

👍 It's all good.


let iconStyle = this.mergeStyles(
const iconStyle = Object.assign({},
Copy link
Member

Choose a reason for hiding this comment

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

Don't worry too much about the let/const. One month ago, I have tried to implement a fix function for the eslint rule. It was almost working. We can almost automate this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome! That's good to know, I definitely support whenever we can reliably automate something 👍

Copy link
Member

Choose a reason for hiding this comment

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

Actually, the pain points are those ones:

    let {
      anchorOrigin,
      className,
      iconButtonElement,
      iconStyle,
      onItemTouchTap,
      onKeyboardFocus,
      onMouseDown,
      onMouseLeave,
      onMouseEnter,
      onMouseUp,
      onTouchTap,
      menuStyle,
      style,
      targetOrigin,
      ...other,
    } = this.props;

When one of the previous variables is used as a let, I can't use const at the top.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think I noticed this too when I was doing it. If I remember correctly it was iconButtonElement that could possibly be reassigned. If we pass the elements that get passed in as props to a stateless internal component, that could handle the logic and we could probably make all of these const then.

Though that process would be tough to automate.

oliviertassinari added a commit that referenced this pull request Feb 5, 2016
[Toggle] Remove style-propable mixin
@oliviertassinari oliviertassinari merged commit 7e64b1d into mui:master Feb 5, 2016
@oliviertassinari
Copy link
Member

@newoga Thanks! You are a beast 🐻.

@newoga
Copy link
Contributor Author

newoga commented Feb 5, 2016

Thanks! 😄

@newoga newoga deleted the #2852/toggle branch February 6, 2016 23:24
@zannager zannager added the component: toggle button This is the name of the generic UI component, not the React module! label Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: toggle button This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants