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

FocalPointPicker: Supply default value as initial percentages state to fix NaN warning #15400

Merged
merged 2 commits into from May 3, 2019

Conversation

@westonruter
Copy link
Member

commented May 2, 2019

The FocalPointPicker component can result in a React warning being emitted from the console:

Warning: Received NaN for the value attribute. If this is expected, cast the value to a string.

The reason for this that the state for the component has an empty percentages by default:

Because of this percentages.x is initially undefined and in this code:

value={ this.fractionToPercentage( percentages.x ) }

The this.fractionToPercentage( percentages.x ) call will return NaN.

The fix seems to be as simple as using the default prop value as the initial state for percentages.

Fixes #15136.

@@ -27,7 +27,7 @@ export class FocalPointPicker extends Component {
this.state = {
isDragging: false,
bounds: {},
percentages: {},
percentages: FocalPointPicker.defaultProps.value,

This comment has been minimized.

Copy link
@aduth

aduth May 2, 2019

Member

It's neither here nor there to the specific fix, but this looks to me like a classic violation of a previously-documented anti-pattern:

http://web.archive.org/web/20160430110308/http://facebook.github.io/react/tips/props-in-getInitialState-as-anti-pattern.html

@@ -27,7 +27,7 @@ export class FocalPointPicker extends Component {
this.state = {
isDragging: false,
bounds: {},
percentages: {},
percentages: FocalPointPicker.defaultProps.value,

This comment has been minimized.

Copy link
@aduth

aduth May 2, 2019

Member

To the specific fix however: It's terribly confusing to me that we assign this value and immediately override it in componentDidMount with a setState.

I think this should just be changed to props.value (props being the first argument of the constructor) and eliminate componentDidMount altogether. Otherwise, we're completely disregarding value from the props for the initial (albeit ultimately wasteful) render.

You may call setState() immediately in componentDidMount(). It will trigger an extra rendering, but it will happen before the browser updates the screen. This guarantees that even though the render() will be called twice in this case, the user won’t see the intermediate state. Use this pattern with caution because it often causes performance issues. In most cases, you should be able to assign the initial state in the constructor() instead. It can, however, be necessary for cases like modals and tooltips when you need to measure a DOM node before rendering something that depends on its size or position.

https://reactjs.org/docs/react-component.html#componentdidmount

(Emphasis mine)

This comment has been minimized.

Copy link
@westonruter

westonruter May 2, 2019

Author Member

Done in 9b1b029

This comment has been minimized.

Copy link
@jeffersonrabb

jeffersonrabb May 2, 2019

Contributor

This looks good to me and tests just fine. Thinking back, I believe I set the percentages in componentDidMount because of an earlier approach to determining the image's dimensions, which I moved away from before submitting the initial Pull Request. Agree with @westonruter it's not necessary.

@swissspidy swissspidy requested a review from aduth May 2, 2019

@aduth

aduth approved these changes May 3, 2019

@aduth aduth merged commit 056b595 into master May 3, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@aduth aduth deleted the fix/focal-point-picker-nan-warning branch May 3, 2019

@youknowriad youknowriad added this to the 5.7 (Gutenberg) milestone May 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.