Pomax
Simplified core
I've simplified the core, removed bunch of possibilities of how the HOC can be used. All of previously covered use cases can be easily implemented in the user land. It has benefits of:
- simpler codebase
- smaller codebase
- being actually more configurable
This would make implementing any custom logic easier - including pending PRs with multiple ignore classes and improved exclude scrollbars logic. Those both PRs could still get merged in, but in a changed/adjusted form.
I have not yet fixed tests (the probably need complete overhaul and I've wanted to present the idea first) and also introduced naming (clickedOutsideComparator and clickedOutsideNodeAndIgnoredSubtree) could get more love.
I would also like to propose HOC factory arguments reordering (function onClickOutsideHOC(config, WrappedComponent)) so creating reusable factories with prefilled config is easier even with simple .bind call.
I am also still unsure why preventDefault and stopPropagation are implemented in the lib itself instead of letting users to handle that. Could you explain this one? Maybe it would be reasonable to remove those too? I don't have strong opinion on this one, although I have always found that a little bit iffy
How to achieve removed instance.props.handleClickOutside feature?
import onClickOutside from 'react-onclickoutside'
onClickOutside(MyComponent, {
handleClickOutside(instance) {
return function(event) {
instance.props.handleClickOutside(event);
};
}
})
How to achieve removed instance.handleClickOutside feature?
import onClickOutside from 'react-onclickoutside'
onClickOutside(MyComponent, {
handleClickOutside(instance) {
return function(event) {
instance.handleClickOutside(event);
};
}
})
How to achieve removed excludeScrollbar feature?
import onClickOutside, { clickedBrowserScrollbar, defaultClickedOutsideComparator } from 'react-onclickoutside'
onClickOutside(MyComponent, {
clickedOutsideComparator(node, event) {
if (clickedBrowserScrollbar(event)) return;
defaultClickedOutsideComparator(node, event)
},
handleClickOutside(instance) {
return someHandler;
}
})
How to achieve removed outsideClickIgnoreClass feature?
import onClickOutside, { clickedOutsideNodeAndIgnoredSubtree } from 'react-onclickoutside'
onClickOutside(MyComponent, {
clickedOutsideComparator(node, event) {
clickedOutsideNodeAndIgnoredSubtree(node, event, 'ignore-react-onclickoutside')
},
handleClickOutside(instance) {
return someHandler;
}
})
The most obvious thing missing in this PR would be the necessary updates to the README.md with a linkback to the "if you're looking for the v6 docs, click here" that links to the README.md from the last v6 tag =)
As for "All of previously covered use cases can be easily implemented in the user land.", we should make the lives of the people who rely on this HOC easier by then also offering a secondary HOC that people can import that works on top of the slimmed down core, giving users all those easily implemented functions as already implemented code, so that the tens of thousands of people who use the HOC right now don't have to reinvent the wheel.
The most obvious thing missing in this PR would be the necessary updates to the README.md with a linkback to the "if you're looking for the v6 docs, click here" that links to the README.md from the last v6 tag =)
Sure, planning to update them before merging this in.
As for "All of previously covered use cases can be easily implemented in the user land.", we should make the lives of the people who rely on this HOC easier by then also offering a secondary HOC that works on top of the slimmed down code that gives users all those easily implemented functions already implement, so that tens of thousands of people don't have to reinvent the wheel, just us.
Providing a second HOC with old features implemented in a really good idea! I'd prefer using simplified version as the default export though as it should cover most use cases.
What do you think about the changes introduced in the PR in general? Also would love your comment about preventDefault/stopPropagation - is there any reason they are controlled by the lib?
I'll give the PR a better lookover later today, but the reason event handling is controlled is because sometimes people don't want to intercept the events, just trigger on them and then let them through. There were some super edge cases that broke by not being able to set them as needed.
One thing that should be fixed in this PR as well is the prepublish command, which uses totally windows-incompatible commands. Also, give that we've been on npm5 for a fair while not, it's worth finally retiring in favour of prepare if we're doing a rewrite like this, too.
Also running into this when trying to run npm test:
08 12 2017 13:30:13.409:INFO [karma]: Karma v1.7.0 server started at http://0.0.0.0:9876/
08 12 2017 13:30:13.414:INFO [launcher]: Launching browser Firefox with unlimited concurrency
08 12 2017 13:30:13.421:INFO [launcher]: Starting browser Firefox
08 12 2017 13:30:17.353:INFO [Firefox 56.0.0 (Windows 10 0.0.0)]: Connected on socket Xb_piU03hqwrfUe7AAAA with id 72092674
08 12 2017 13:30:27.763:WARN [Firefox 56.0.0 (Windows 10 0.0.0)]: Disconnected (1 times), because no message in 10000 ms.
Firefox 56.0.0 (Windows 10 0.0.0) ERROR
Disconnected, because no message in 10000 ms.
Running a build and then using a server to load up the browser based test also results in a non-functional page, so we definitely need to fix testing for this PR.
I didn't notice this before, but there is a piece of code:
events.forEach(eventName => {
let handlerOptions = null;
const isTouchEvent = touchEvents.indexOf(eventName) !== -1;
if (isTouchEvent && passiveEventSupport) {
handlerOptions = { passive: !this.props.preventDefault };
}
document.addEventListener(eventName, DOMHandlersMap[this._uid], handlerOptions);
});
which declares a const which is then only used once, despite the value being static, so this feels a little too "DRY" - there is no repetition yet, no need to factor it out. In this case, rather than using touchEvents.indexOf(eventName) !== -1 we can use touchEvents.includes(eventName), which is plain a boolean check.
One thing that should be fixed in this PR as well is the prepublish command, which uses totally windows-incompatible commands. Also, give that we've been on npm5 for a fair while not, it's worth finally retiring in favour of prepare if we're doing a rewrite like this, too.
I'm totally fine with migrating to prepare completely. 👍
Running a build and then using a server to load up the browser based test also results in a non-functional page, so we definitely need to fix testing for this PR.
Oh yeah, this PR is not merge-ready yet. My intention was proposing a simplified API as an idea. While it's probably functional already, tests & readme definitely need to be updated. I wanted only to get initial code/idea review, ignoring the rest that is not finished.
which declares a const which is then only used once, despite the value being static, so this feels a little too "DRY" - there is no repetition yet, no need to factor it out. In this case, rather than using touchEvents.indexOf(eventName) !== -1 we can use touchEvents.includes(eventName), which is plain a boolean check.
It's not even matter of DRY here, but rather a readability issue - identifier with a given name in such a case is imho more readable.
As to includes - it's part of es2017 and we aim for es5 compatibility.