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

feat: improve useClickAway() hook #394

Merged
merged 2 commits into from
Jun 19, 2019
Merged

feat: improve useClickAway() hook #394

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3305f39
save callback independently
jeetiss Jun 14, 2019
8fb5895
don't use alert in story
jeetiss Jun 14, 2019
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: 1 addition & 1 deletion docs/useClickAway.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {useClickAway} from 'react-use';
const Demo = () => {
const ref = useRef(null);
useClickAway(ref, () => {
alert('OUTSIDE CLICKED');
console.log('OUTSIDE CLICKED');
});

return (
Expand Down
5 changes: 2 additions & 3 deletions src/__stories__/useClickAway.story.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { action } from '@storybook/addon-actions';
import { storiesOf } from '@storybook/react';
import * as React from 'react';
import { useRef } from 'react';
Expand All @@ -6,9 +7,7 @@ import ShowDocs from './util/ShowDocs';

const Demo = () => {
const ref = useRef(null);
useClickAway(ref, () => {
alert('OUTSIDE CLICKED');
});
useClickAway(ref, action('outside clicked'));

return (
<div
Expand Down
10 changes: 7 additions & 3 deletions src/useClickAway.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { RefObject, useEffect } from 'react';
import { RefObject, useEffect, useRef } from 'react';
import { off, on } from './util';

const defaultEvents = ['mousedown', 'touchstart'];
Expand All @@ -8,10 +8,14 @@ const useClickAway = (
onClickAway: (event: KeyboardEvent) => void,
events: string[] = defaultEvents
) => {
const savedCallback = useRef(onClickAway);
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of saving the callback inside the useClickAway hook, isn't it better if consumer do it manually?

const onClickAway = useCallback(() => { /* ... */ });
useClickAway(ref, onClickAway);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't want to write extra code on every useClickAway usage
and this approach is less effective

Copy link

Choose a reason for hiding this comment

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

may I ask what is the purpos of using the useRef here ?
why not simply use the callback ?

useEffect(() => {
savedCallback.current = onClickAway;
}, [onClickAway]);
useEffect(() => {
const handler = event => {
const { current: el } = ref;
el && !el.contains(event.target) && onClickAway(event);
el && !el.contains(event.target) && savedCallback.current(event);
};
for (const eventName of events) {
on(document, eventName, handler);
Expand All @@ -21,7 +25,7 @@ const useClickAway = (
off(document, eventName, handler);
}
};
});
}, [events, ref]);
Copy link
Owner

Choose a reason for hiding this comment

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

events is an array, I though array cannot be a dep, maybe it should be like this:

[...events, ref]

Copy link
Contributor Author

@jeetiss jeetiss Jun 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

you're right 🤗

https://codesandbox.io/s/deps-array-kj8ov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, I'm trying to figure it out myself.

And you approach is totally wrong, with spread operator in deps react writes this warnings


Warning: The final argument passed to useEffect changed size between renders. The order and size of this array must remain constant.

Previous: [1, 2, 3, 1, 1, 1]
Incoming: [1, 2, 3]
    in App

Copy link
Owner

Choose a reason for hiding this comment

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

Could useEffect be changed to useDeepCompareEffect?

Copy link
Contributor Author

@jeetiss jeetiss Jun 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Why? useEffect works fine with array!
See codesandbox example

Copy link
Owner

@streamich streamich Jun 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I guess you are right. I was thinking if events is provided inline, it will be a new array every re-render.

useClickAway(ref, fn, ['mousedown', 'touchstart'])

User needs to make sure events is memoized.

};

export default useClickAway;