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

[docs] Fix popover demo event.target is null #17104

Merged

Conversation

spaceexperiment
Copy link
Contributor

@spaceexperiment spaceexperiment commented Aug 22, 2019

Closes #17101

@spaceexperiment spaceexperiment mentioned this pull request Aug 22, 2019
2 tasks
@mui-pr-bot
Copy link

No bundle size changes comparing 343ae8e...44864d2

Generated by 🚫 dangerJS against 44864d2

@mbrookes mbrookes added the component: Popper The React component. See <Popup> for the latest version. label Aug 22, 2019
@oliviertassinari oliviertassinari changed the title [docs] fix popover demo event.target is null [docs] Fix popover demo event.target is null Aug 22, 2019
@@ -99,9 +99,10 @@ function AnchorPlayground(props) {
};

const handleNumberInputChange = key => event => {
const value = event.target.value;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is better:
const {value} = event.target;

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that it makes a difference. I believe that the convention in the codebase is to not destructure the two options are equivalent.

Choose a reason for hiding this comment

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

How did this PR fix a bug? As far as I can see, there is no functional difference in the new code and old code; if target is null, it will still be dereferenced and cause a crash.

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Yeap, let's change the approach this time!

@oliviertassinari oliviertassinari merged commit 2bfa8a0 into mui:master Aug 23, 2019
@oliviertassinari
Copy link
Member

@spaceexperiment Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Popper The React component. See <Popup> for the latest version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Popover Demo runtime error
6 participants