[eslint] Enforce jsx-handler-names #3408

oliviertassinari · 2016-02-21T10:48:22Z

This one is for @nathanmarks.
I have done some aggressive clean up. Be careful when reviewing 🚨.

nathanmarks · 2016-02-21T17:37:29Z

@oliviertassinari I wish we had tests for all these handlers right now!

Something to note, unfortunately the eslint rule is only for JSX. Which means there is an opportunity for inconsistency. See here: https://github.com/oliviertassinari/material-ui/blob/eslint-jsx-handler-names/src/DropDownMenu/DropDownMenu.jsx#L286

Maybe a quick search in /src/ for _on is in order? Not sure if you want to enforce the same convention with non-jsx elements, but it'd make sense imo.

oliviertassinari · 2016-02-21T19:26:08Z

I wish we had tests for all these handlers right now!

Me too. Don't worry, my agressive changes are regarding using the open state to speedup the DatePicker and the TimePicker.

Not sure if you want to enforce the same convention with non-jsx elements, but it'd make sense imo.

I agree with you but the conflict potential of this PR with others is already high. I would say, let's take care of this with another PR.

alitaheri · 2016-02-21T19:39:45Z

Farewell ugly underscore 😜 👍

nathanmarks · 2016-02-21T20:08:48Z

@alitaheri React components full of underscore dangle'd class methods is the stuff of my nightmares. Readability is so much better with these changes @oliviertassinari !!

oliviertassinari · 2016-02-27T01:00:38Z

Ping me when you think I should rebase.

nathanmarks · 2016-02-27T01:32:06Z

@oliviertassinari Outside of what I mentioned initially, it looked good to me. Getting this merged would be great, I almost posted an issue by accident today regarding inconsistent use of comma dangled event handler methods 😄

oliviertassinari · 2016-02-27T11:41:47Z

it looked good to me. Getting this merged would be great

😄. This PR may have conflict with many others PRs. I think that we should apply this change and the ES6 classes migration in a row.

newoga · 2016-02-27T17:41:50Z

I think that we should apply this change and the ES6 classes migration in a row.

@oliviertassinari I agree!

nathanmarks · 2016-02-27T17:54:55Z

@newoga @oliviertassinari

Is there a PR for that? I can help combine them before merging to master if you'd like

oliviertassinari · 2016-02-27T17:58:50Z

Is there a PR for that? I can help combine them before merging to master if you'd like

By in a row, I was meaning at the same time, like the same day, not combined 😁. We merge this one, then, we run the codemod for the ES6 classe.
The question is, when do we do this move? Let's try to minimize the impact.

newoga · 2016-02-27T18:07:44Z

The question is, when do we do this move?

I know this sounds crazy but I'm thinking we should save both of these for right before we do the directory restructure. Those three probably represent the biggest code changes in this release that will impact existing PRs.

I'd say do this order:

Tag open PRs that we plan to have fixed for 0.15.0.
Merge those PRs
Apply remaining eslint rules
Run react-codemod to convert to ES6 and replace PureRenderMixin
Reorganize directories

I would like to do steps 3, 4, and 5 in a phase when we aren't merging any other PRs and we can do it in a short time span (same day like @oliviertassinari mentioned).

After that, existing PRs can be resubmitted or updated against the new and moved ES6 class based components.

Edit: It sounds like a lot of work but I think we're pretty close. I think the biggest step is reviewing existing PRs and getting them merged before the migration.

nathanmarks · 2016-02-27T18:33:26Z

@newoga That sounds sensible.

When we finally "do it", might want to set it up on a temporary branch before merging it into master.

mbrookes · 2016-02-28T11:49:57Z

I think the biggest step is reviewing existing PRs and getting them merged before the migration.

I agree. And while we've been doing a better job with new PRs, do you think perhaps we should take a break from refactoring, and all double-down on catching up with the PR backlog?

I know it's painful, but the longer we leave it, the harder fixing merge conflicts becomes.

newoga · 2016-02-28T18:06:39Z

break from refactoring, and all double-down on catching up with the PR backlog?

Yeah I think this is a good idea. I think we're just going to have to focus on the existing PRs and decide which ones will get in 0.15.0 and which ones have to get resubmitted after the ES6 and linting migrations.

@oliviertassinari @alitaheri, are you good with this?

oliviertassinari · 2016-02-28T18:20:19Z

on catching up with the PR backlog

That sounds like an good idea.

and which ones have to get resubmitted after the ES6 and linting migrations.

We have different types of pending PRs. I think that they needs different care:

Some are not relevant: let's close them
Some needs a minor rebase: we can or the author can take care of it.
Some needs work and the author is still active, we can merge them for the 0.15.0 release
Some needs work and the author is not longer active: we can take care of them later.

alitaheri · 2016-02-28T18:29:34Z

That makes sense 👍 I think we should take care of the backlog. It's getting bloated and scary 😨

mbrookes · 2016-03-23T00:33:04Z

@oliviertassinari - is now the time rebase and apply this? (And any other desired eslint rules?)

oliviertassinari · 2016-03-23T07:44:57Z

@mbrookes That sounds like the right moment. I will try to rebase before the ES6 classes conversion.

oliviertassinari · 2016-03-25T21:33:02Z

docs/src/app/components/pages/components/Table/ExampleComplex.js

@@ -86,7 +86,6 @@ export default class TableExampleComplex extends React.Component {
          fixedFooter={this.state.fixedFooter}
          selectable={this.state.selectable}
          multiSelectable={this.state.multiSelectable}
-          onRowSelection={this._onRowSelection}


_onRowSelection is missing.

oliviertassinari · 2016-03-25T23:19:40Z

I have just rebased this PR.

oliviertassinari added PR: Needs Review labels Feb 21, 2016

oliviertassinari mentioned this pull request Feb 21, 2016

[Core] Improve eslint a bit more #2903

Closed

20 tasks

oliviertassinari added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Feb 27, 2016

mbrookes added PR: out-of-date The pull request has merge conflicts and can't be merged on hold There is a blocker, we need to wait and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Mar 2, 2016

mbrookes assigned nathanmarks Mar 2, 2016

oliviertassinari added this to the 0.16.0 Release milestone Mar 6, 2016

oliviertassinari removed the on hold There is a blocker, we need to wait label Mar 23, 2016

mbrookes added this to the 0.15.0-beta.1 Release milestone Mar 23, 2016

mbrookes removed this from the 0.16.0 Release milestone Mar 23, 2016

oliviertassinari reviewed Mar 25, 2016
View reviewed changes

[eslint] Enforce jsx-handler-names

6494b28

oliviertassinari added PR: Needs Review and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Mar 25, 2016

oliviertassinari added the Refactoring label Mar 26, 2016

mbrookes added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Mar 26, 2016

oliviertassinari added PR: Needs Review and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Mar 26, 2016

[EnhancedSwitch] onParentShouldUpdate is no longer mandatory

e5be36b

mbrookes added PR: Review Accepted and removed PR: Needs Review labels Mar 27, 2016

mbrookes merged commit 4b1b039 into mui:master Mar 27, 2016

oliviertassinari deleted the eslint-jsx-handler-names branch March 27, 2016 00:44

mbrookes mentioned this pull request Mar 28, 2016

[Core] Remove default theme handling from components #3820

Merged

zannager added package: system Specific to @mui/system package: eslint Specific to eslint-plugin-material-ui labels Mar 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[eslint] Enforce jsx-handler-names #3408

[eslint] Enforce jsx-handler-names #3408

oliviertassinari commented Feb 21, 2016

nathanmarks commented Feb 21, 2016

oliviertassinari commented Feb 21, 2016

alitaheri commented Feb 21, 2016

nathanmarks commented Feb 21, 2016

oliviertassinari commented Feb 27, 2016

nathanmarks commented Feb 27, 2016

oliviertassinari commented Feb 27, 2016

newoga commented Feb 27, 2016

nathanmarks commented Feb 27, 2016

oliviertassinari commented Feb 27, 2016

newoga commented Feb 27, 2016

nathanmarks commented Feb 27, 2016

mbrookes commented Feb 28, 2016

newoga commented Feb 28, 2016

oliviertassinari commented Feb 28, 2016

alitaheri commented Feb 28, 2016

mbrookes commented Mar 23, 2016

oliviertassinari commented Mar 23, 2016

oliviertassinari Mar 25, 2016

oliviertassinari commented Mar 25, 2016

[eslint] Enforce jsx-handler-names #3408

[eslint] Enforce jsx-handler-names #3408

Conversation

oliviertassinari commented Feb 21, 2016

nathanmarks commented Feb 21, 2016

oliviertassinari commented Feb 21, 2016

alitaheri commented Feb 21, 2016

nathanmarks commented Feb 21, 2016

oliviertassinari commented Feb 27, 2016

nathanmarks commented Feb 27, 2016

oliviertassinari commented Feb 27, 2016

newoga commented Feb 27, 2016

nathanmarks commented Feb 27, 2016

oliviertassinari commented Feb 27, 2016

newoga commented Feb 27, 2016

nathanmarks commented Feb 27, 2016

mbrookes commented Feb 28, 2016

newoga commented Feb 28, 2016

oliviertassinari commented Feb 28, 2016

alitaheri commented Feb 28, 2016

mbrookes commented Mar 23, 2016

oliviertassinari commented Mar 23, 2016

oliviertassinari Mar 25, 2016

Choose a reason for hiding this comment

oliviertassinari commented Mar 25, 2016