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

Components: refactorFocalPointPicker to pass exhaustive-deps #41520

Merged
merged 9 commits into from
Jul 4, 2022
Merged

Components: refactorFocalPointPicker to pass exhaustive-deps #41520

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b7c7d96
FocalPointPicker: add `pan` to `useEffect` dep array
chad1008 Jun 2, 2022
ac030fc
refactor `useRef` calls for both `locationPageOffset` variables
chad1008 Jun 2, 2022
8eacfe4
add `pan`, `onChange`, and `shouldEnableBottomSheetScroll` to `panRes…
chad1008 Jun 2, 2022
c9ffb72
FocalPointPicker: add `startAnimation` to `useEffect` dep array. Wrap…
chad1008 Jun 2, 2022
2078780
FocalPointPicker: update changelog
chad1008 Jun 2, 2022
b89fbb2
move `startAnimation` directly into `useEffect`
chad1008 Jun 8, 2022
63cd003
FocalPointPicker: wrap `FocalPointSettingsPanelMemo`s `onChange` in a…
chad1008 Jun 15, 2022
d73ee30
FocalPointPicker: fix typo to ensure `setPosition` is actually invoked
chad1008 Jun 15, 2022
3ebaf4a
Update packages/components/src/mobile/focal-point-settings-panel/inde…
chad1008 Jun 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
- `Scrollable`: Convert to TypeScript ([#42016](https://github.com/WordPress/gutenberg/pull/42016)).
- `Spacer`: Complete TypeScript migration ([#42013](https://github.com/WordPress/gutenberg/pull/42013)).
- `TreeSelect`: Refactor away from `_.repeat()` ([#42070](https://github.com/WordPress/gutenberg/pull/42070/)).
- `FocalPointPicker` updated to satisfy `react/exhuastive-deps` eslint rule ([#41520](https://github.com/WordPress/gutenberg/pull/41520)).

## 19.14.0 (2022-06-29)

Expand Down
16 changes: 8 additions & 8 deletions packages/components/src/focal-point-picker/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ function FocalPointPicker( props ) {
const [ videoNaturalSize, setVideoNaturalSize ] = useState( null );
const [ tooltipVisible, setTooltipVisible ] = useState( false );

let locationPageOffsetX = useRef().current;
let locationPageOffsetY = useRef().current;
const locationPageOffsetX = useRef();
const locationPageOffsetY = useRef();
const videoRef = useRef( null );

useEffect( () => {
Expand All @@ -67,7 +67,7 @@ function FocalPointPicker( props ) {
y: focalPoint.y * containerSize.height,
} );
}
}, [ focalPoint, containerSize ] );
}, [ focalPoint, containerSize, pan ] );

// Pan responder to manage drag handle interactivity.
const panResponder = useMemo(
Expand All @@ -86,8 +86,8 @@ function FocalPointPicker( props ) {
pageX,
pageY,
} = event.nativeEvent;
locationPageOffsetX = pageX - x;
locationPageOffsetY = pageY - y;
locationPageOffsetX.current = pageX - x;
locationPageOffsetY.current = pageY - y;
pan.setValue( { x, y } ); // Set cursor to tap location.
pan.extractOffset(); // Set offset to current value.
},
Expand All @@ -106,8 +106,8 @@ function FocalPointPicker( props ) {
// Specifically, dragging the handle outside the bounds of the image
// results in inaccurate locationX and locationY coordinates to be
// reported. https://github.com/facebook/react-native/issues/15290#issuecomment-435494944
const x = pageX - locationPageOffsetX;
const y = pageY - locationPageOffsetY;
const x = pageX - locationPageOffsetX.current;
const y = pageY - locationPageOffsetY.current;
onChange( {
x: clamp( x / containerSize?.width, 0, 1 ).toFixed( 2 ),
y: clamp( y / containerSize?.height, 0, 1 ).toFixed(
Expand All @@ -120,7 +120,7 @@ function FocalPointPicker( props ) {
setSliderKey( ( prevState ) => prevState + 1 );
},
} ),
[ containerSize ]
[ containerSize, pan, onChange, shouldEnableBottomSheetScroll ]
Copy link
Member

Choose a reason for hiding this comment

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

It looks like onChange is changing on every change that's happening on the Focal point picker when updating the sliders and during the touch events, making the panResponder to be created constantly.

This prop is coming from here. So I think we should add a useCallback there (for setPosition) to prevent this from happening.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @geriux! Let me know if e963825 helps improve things! 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually scratch that. There's a typo in this commit that was preventing the linter from detecting further issues. Stand by! 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, fixed again in 86ba326.

One odd wrinkle is that exhaustive-deps doesn't seem to mind the empty dep array, when I'd have expected it to want us to add setPosition as a dependency. If we do add it, we get a warning from the linter (unsurprisingly) that it's changing on every render.

I'm thinking it might be that the linter can detect that setPosition just calls setDraftFocalPoint, which in turn comes from useState and will therefor never change. My understanding is that ESLint is smart enough not to require state-setters as deps, but I didn't realize it could parse through an additional layer of abstraction like this (assuming I'm understanding things correctly here).

Copy link
Member

Choose a reason for hiding this comment

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

It looks like onChange is changing on every change that's happening on the Focal point picker when updating the sliders and during the touch events, making the panResponder to be created constantly.

@geriux good catch! How did you come across the changing onChange value? Did you notice a performance issue or observe it in a different manner? Primarily curious to improve my ability to identify this type of thing in the future.

);

const mediaBackground = usePreferredColorSchemeStyle(
Expand Down
21 changes: 10 additions & 11 deletions packages/components/src/focal-point-picker/tooltip/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,18 +66,17 @@ function Label( { align, text, xOffset, yOffset } ) {
}

useEffect( () => {
const startAnimation = () => {
Animated.timing( animationValue, {
toValue: visible ? 1 : 0,
duration: visible ? 300 : 150,
useNativeDriver: true,
delay: visible ? 500 : 0,
easing: Easing.out( Easing.quad ),
} ).start();
};
startAnimation();
}, [ visible ] );

const startAnimation = () => {
Animated.timing( animationValue, {
toValue: visible ? 1 : 0,
duration: visible ? 300 : 150,
useNativeDriver: true,
delay: visible ? 500 : 0,
easing: Easing.out( Easing.quad ),
} ).start();
};
}, [ animationValue, visible ] );

// Transforms rely upon onLayout to enable custom offsets additions.
let tooltipTransforms;
Expand Down
4 changes: 2 additions & 2 deletions packages/components/src/mobile/focal-point-settings-panel/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { useRoute, useNavigation } from '@react-navigation/native';
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { memo, useContext, useState } from '@wordpress/element';
import { memo, useContext, useState, useCallback } from '@wordpress/element';
import { BottomSheetContext, FocalPointPicker } from '@wordpress/components';

/**
Expand Down Expand Up @@ -56,7 +56,7 @@ const FocalPointSettingsPanelMemo = memo(
</NavBar>
<FocalPointPicker
focalPoint={ draftFocalPoint }
onChange={ setPosition }
onChange={ useCallback( setPosition, [] ) }
shouldEnableBottomSheetScroll={
shouldEnableBottomSheetScroll
}
Expand Down