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

[ExpansionPanelSummary] Fix event forwarding #13582

Merged
merged 2 commits into from
Nov 13, 2018

Conversation

jmetev1
Copy link
Contributor

@jmetev1 jmetev1 commented Nov 13, 2018

Closes #13577 and adds test for it.

I wasn't clear on what I should change proptypes to. onchange and onclick are in proptypes now, but they're ignored from the generated docs. Is that done for some reason? Should onfocus and onblur be added there but with @ignore for some reason? I can update it to whatever is desired style.

@jmetev1 jmetev1 changed the title expansions summary handleblur now calls onblur [ExpansionPanelSummary] handleblur now calls onblur Nov 13, 2018
@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work component: accordion This is the name of the generic UI component, not the React module! labels Nov 13, 2018
@oliviertassinari oliviertassinari changed the title [ExpansionPanelSummary] handleblur now calls onblur [ExpansionPanelSummary] Fix event forwarding Nov 13, 2018
@oliviertassinari
Copy link
Member

@jmetev1 It's a great first pull request on Material-UI 👌🏻. Thank you for working on it!

@oliviertassinari
Copy link
Member

Is that done for some reason?

We avoid documenting the native properties, it would be noise to our users.

@oliviertassinari
Copy link
Member

Should onfocus and onblur be added there but with @ignore for some reason? I can update it to whatever is desired style.

Good question, we used to have an eslint rule that ask for defining all the proptypes of the properties we use. It seems that eslitn rule is no longer working 😱. It's this rule to be specific: https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/prop-types.md. Do you want to look into this problem?

@jmetev1
Copy link
Contributor Author

jmetev1 commented Nov 14, 2018

thank you. I'm looking at eslint rule problem now. looks like some kind of conflict with another rule. trying to narrow it down now. @oliviertassinari

@oliviertassinari
Copy link
Member

@jmetev1 Let us know if you can find anything! I doubt we are the only one affected.

@jmetev1
Copy link
Contributor Author

jmetev1 commented Nov 16, 2018

Well i now know far more than i used to about eslint config. So this has been talked about already jsx-eslint/eslint-plugin-react#1958 So package.json had "eslint-plugin-react ^7.4.0". And yarn.lock had 7.11.1, which is the latest. So that's what I was working with. In that link the eslnt-plugin-react people say it's fixed but they haven't published. The problem appears to be use of spread operators. And i confirmed that when I made a simple component with no spread operators, failure to document prop types did get correctly flagged when using 7.11.1. So since they said it was published, I grabbed the newest copy of repo from their repo and dropped it in to node modules. And now it correctly flags props that are used but not documented even with spread operator. As mentioned in that link, another work around is dropping back to 7.10.0. But that causes some problems with eslint airbnb package. And eslint-pulgin-react people haven't published since august. Someone even asked jsx-eslint/eslint-plugin-react#2046 and the answer wasn't super encouraging. Soooo. I don't see much of any solution unless someone wants to fork the eslint-plugin-react repo and point package.json to that directly or something messy like that. Thanks for reading my novel! @oliviertassinari

@oliviertassinari
Copy link
Member

@jmetev1 I believe our setup is pretty common, let's wait. I hope they will sort the issue out. Thank you for the investigation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: accordion 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.

2 participants