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

[eslint] Refactor some component to use const instead of let #3300

Merged
merged 1 commit into from
Feb 12, 2016
Merged

[eslint] Refactor some component to use const instead of let #3300

merged 1 commit into from
Feb 12, 2016

Conversation

oliviertassinari
Copy link
Member

I have used the auto-fix option of eslint with this homemade fix function:

fix: function(fixer) {
  return fixer.replaceTextRange([declaration.start, declaration.start + 3], "const");
}

I hade to refactor some component too. Be careful when reviewing 😁.

newoga
newoga reviewed Feb 11, 2016
View reviewed changes
src/app-bar.jsx
switch (iconElementLeft.type.displayName) {
case 'IconButton':
iconElementLeft = React.cloneElement(iconElementLeft, {
iconElementLeftNode = React.cloneElement(iconElementLeft, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the auto fixer be making this change?

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay I see, I suppose you made some minor manual fixes along the way.

@newoga
Copy link
Contributor

newoga commented Feb 11, 2016

Looks good to me 👍

This PR makes me really happy.

happyness

@alitaheri
Copy link
Member

This is awesome 😍

However, this PR touches a lot of files as it is. Any single PR merge is gonna conflict 😁

@oliviertassinari
Copy link
Member Author

Any single PR merge is gonna conflict

It's already true ><. Alright, I'm gonna do it in two steps.
In this PR, I'm gonna only apply the manual changes that only affects 3 or 4 files. Once the first PR is merged. I will wait for the right moment to apply the auto fix.

@alitaheri
Copy link
Member

@oliviertassinari That sounds great 👍 Thanks a lot 👍 👍

@oliviertassinari oliviertassinari changed the title [eslint] Enforce prefer-const [eslint] Refactor some component to use const instead of let Feb 12, 2016
@oliviertassinari
Copy link
Member Author

That's good to be reviewed again. I feel like I will have to enable the #ninja mode for the second PR 💥. There is a lot of PR merged every day.

@newoga
Copy link
Contributor

newoga commented Feb 12, 2016

That looks good 👍

@alitaheri alitaheri closed this Feb 12, 2016
@alitaheri alitaheri reopened this Feb 12, 2016
@alitaheri
Copy link
Member

This is great thanks a lot 👍 👍

alitaheri added a commit that referenced this pull request Feb 12, 2016
@alitaheri
Merge pull request #3300 from oliviertassinari/eslint-prefer-const-2
96e106b 
[eslint] Refactor some component to use const instead of let
@alitaheri alitaheri merged commit 96e106b into mui:master Feb 12, 2016
@oliviertassinari oliviertassinari deleted the eslint-prefer-const-2 branch February 12, 2016 15:09
@oliviertassinari oliviertassinari mentioned this pull request Feb 12, 2016
Closed
20 tasks
@zannager zannager added package: system Specific to @mui/system package: eslint Specific to eslint-plugin-material-ui labels Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
package: eslint Specific to eslint-plugin-material-ui package: system Specific to @mui/system
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@oliviertassinari @newoga @alitaheri @zannager