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

Components: refactorDraggable to pass exhaustive-deps #41499

Merged
merged 4 commits into from
Jun 8, 2022
+3 −1
Merged

Components: refactorDraggable to pass exhaustive-deps #41499

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2b3229a
Draggable: update `useEffect` dependency array
chad1008 Jun 2, 2022
8567280
Draggable: update changelog (merge conflict)
chad1008 Jun 2, 2022
4333607
move changelog update to the correct section
chad1008 Jun 8, 2022
5504b8d
Merge branch 'trunk' into enhance/Draggable-exhuastive-deps
chad1008 Jun 8, 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
2 changes: 2 additions & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,13 @@
### Internal

- `FormTokenField`: Convert to TypeScript and refactor to functional component ([#41216](https://github.com/WordPress/gutenberg/pull/41216)).
- `Draggable`: updated to satisfy `react/exhuastive-deps` eslint rule ([#41499](https://github.com/WordPress/gutenberg/pull/41499))
- `RadioControl`: Convert to TypeScript ([#41568](https://github.com/WordPress/gutenberg/pull/41568)).
- `Flex` updated to satisfy `react/exhuastive-deps` eslint rule ([#41507](https://github.com/WordPress/gutenberg/pull/41507)).
- `CustomGradientBar` updated to satisfy `react/exhuastive-deps` eslint rule ([#41463](https://github.com/WordPress/gutenberg/pull/41463))
- `TreeSelect`: Convert to TypeScript ([#41536](https://github.com/WordPress/gutenberg/pull/41536)).


## 19.12.0 (2022-06-01)

### Bug Fix
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/draggable/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ const Draggable = ( {

const providerValue = useMemo( () => {
return { panGestureRef, isDragging, isPanActive, draggingId };
}, [] );
}, [ isDragging, isPanActive, draggingId ] );
Copy link
Contributor

Choose a reason for hiding this comment

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

isDragging, isPanActive and draggingId are shared value object types created using the hook useSharedValue provided by the React Native's Reanimated library.

gutenberg/packages/components/src/draggable/index.native.js

Lines 48 to 50 in 3512a38

const isDragging = useSharedValue( false );
const isPanActive = useSharedValue( false );
const draggingId = useSharedValue( '' );

The shared values are similar to the React references in the sense that their value doesn't update after they are initialized. Therefore, it's not really necessary to add them as dependencies of useMemo because the value won't change. However, I understand that since this type is not recognized by the exhuastive-deps ESLint rule, we could add them in order to comply with it 👍 .

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 @fluiddot!

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you both!

Maybe we could leave an inline comment in code, so that the next time folks look at this line of code they have more context about isDragging, isPanActive and draggingId ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep good point, we could leave a comment when using the shared value objects in hooks to outline that they are not required to be added in the dependencies array 👍 .

Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny follow-up opened in #41658


return (
<GestureDetector gesture={ panGesture }>
Expand Down