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

Fix closing of calendar on mobile scroll #1736

wants to merge 6 commits into
base: master


Copy link

commented May 10, 2019

Fixes #512, relevant react-onclickoutside/#263.

react-onclickoutside uses the mousedown and touchstart events. This means that they also trigger on mobile scroll (because that requires touching the screen). This PR updates that to use click instead.

Using click exposes some fragile code relating to the blur event of the input. This is intended to re-focus the input when the user clicks inside the calendar (e.g. clicks on the year dropdown), but it never actually checks where the blur event originates from. This worked before because the outsideclick triggered before the blur, but now that it uses click instead it's the other way around. This PR also updates that code to check if the event originates from a click on the calendar.

birjolaxew added 2 commits May 10, 2019
By default react-onclickoutside uses `touchstart` and `mousedown`
This means that it closes on e.g. mobile scroll
This changes that to use `click`, which should only happen on actual click
The input is re-focused whenever something inside the calendar is clicked
This was a bit over-eager, and didn't actually check if the calendar was clicked
Now that the outsideclick listener uses `click` instead of `mousedown` (069eccb)
  this became apparent because the blur happens before the outsideclick
Fixed it to now check if the blur originated from a click inside calendar
@birjolaxew birjolaxew force-pushed the birjolaxew:fix/512 branch from 7808054 to c1341be May 10, 2019
@birjolaxew birjolaxew force-pushed the birjolaxew:fix/512 branch from c1341be to 1e8147b May 10, 2019

This comment has been minimized.

Copy link

commented May 10, 2019

Codecov Report

Merging #1736 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1736      +/-   ##
+ Coverage   90.32%   90.35%   +0.02%     
  Files          17       17              
  Lines        1209     1223      +14     
  Branches      208      209       +1     
+ Hits         1092     1105      +13     
- Misses         10       11       +1     
  Partials      107      107
Impacted Files Coverage Δ
src/calendar.jsx 90% <ø> (+0.19%) ⬆️
src/calendar_container.jsx 70% <100%> (+7.5%) ⬆️
src/index.jsx 91.01% <100%> (-0.11%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65a4fb2...e06dd2c. Read the comment docs.

@birjolaxew birjolaxew force-pushed the birjolaxew:fix/512 branch from 2dd208a to 1e8147b May 10, 2019
birjolaxew and others added 3 commits May 16, 2019
The test was failing due to testing re-focusing without triggering touchstart
This is a more robust fix than the now-reverted e0e5323

The issue is caused by the move from touchstart to click for clickoutside
react-onclickoutside ignores events when the ignore class is set
Before the change this was ok because react-onclickoutside listened before the class was applied
After the change the class is applied before the click event is triggered, so the click is ignored
This means that the original datepicker is never told to close when another input is clicked

Now removes the usage of an ignore class entirely (except for the public-facing API)
Instead makes a manual check if the event target is something we want to ignore
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
1 participant
You can’t perform that action at this time.