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 IE11 autoscroll behaviour #1308

Closed

Conversation

nathanpower
Copy link

@nathanpower nathanpower commented May 15, 2019

Repro:

  • Add a console.log(action.type) here
  • Run the storybook and go to fixed list with fixed sidebar
  • (In Chrome) with dev-tools open, drag an item such that auto-scroll is triggered, note action types logged
  • (In IE11) with dev-tools open, drag an item such that auto-scroll is triggered, note action types logged

Bug (IE11 only):

  • Error in console - Invariant failed: Window scrolling is currently not supported for fixed lists. Aborting drag
  • MOVE_BY_WINDOW_SCROLL action dispatched (whereas it is not in Chrome)

I am not sure I have the full context of how window scrolling is managed here, so this could be the wrong approach, but in any case I suspect that the fix in #1088 did not have the intended effect.

Happy to look for other solutions if this is not the way to go.

Fantastic library and API, many thanks for your efforts 👏

@alexreardon
Copy link
Collaborator

Thanks for this. We are in the middle of changing the event system. It would be good to have this change go into that. I'll post details soon so we can retarget this

@alexreardon
Copy link
Collaborator

But maybe we could just add this and port it..

@alexreardon
Copy link
Collaborator

Can you please check [email protected] to see if that fixes this issue?

@nathanpower
Copy link
Author

Can you please check [email protected] to see if that fixes this issue?

I get the same error using [email protected] @alexreardon, everything else seems to be working well though

@alexreardon
Copy link
Collaborator

I will look at this for 12.0 #1317

@alexreardon
Copy link
Collaborator

I won't merge this as is as the keyboard sensor has changed completely.

Can you please retarget this PR on the virtual branch? Then we can ship it easily :)

@nathanpower
Copy link
Author

Can you please retarget this PR on the virtual branch?

Sure no problem.

Having some issues though, should I be able to build that branch locally?

If not, maybe I'll wait until the build is passing to raise PR. Will be easier to try add coverage etc.?

Build error ➜ react-beautiful-dnd git:(virtual) yarn && yarn build yarn install v1.16.0 [1/4] 🔍 Resolving packages... [2/4] 🚚 Fetching packages... [3/4] 🔗 Linking dependencies... warning " > [email protected]" has incorrect peer dependency "eslint@^4.19.1 || ^5.3.0". warning "eslint-config-airbnb > [email protected]" has incorrect peer dependency "eslint@^4.19.1 || ^5.3.0". warning " > [email protected]" has incorrect peer dependency "eslint@^3 || ^4 || ^5". [4/4] 🔨 Building fresh packages... ✨ Done in 18.80s. yarn run v1.16.0 $ yarn build:clean && yarn build:dist && yarn build:flow $ rimraf dist $ rollup -c

./src/index.js → dist/react-beautiful-dnd.js...
[!] Error: 'isValidElementType' is not exported by node_modules/react-is/index.js
https://rollupjs.org/guide/en#error-name-is-not-exported-by-module-
node_modules/react-redux/es/components/connectAdvanced.js (6:9)
4: import invariant from 'invariant';
5: import React, { useContext, useMemo, useEffect, useLayoutEffect, useRef, useReducer } from 'react';
6: import { isValidElementType, isContextConsumer } from 'react-is';
^
7: import Subscription from '../utils/Subscription';
8: import { ReactReduxContext } from './Context'; // Define some constant arrays just to avoid re-creating these
Error: 'isValidElementType' is not exported by node_modules/react-is/index.js
at error (/Users/nathan_power/workspaces/personal/react-beautiful-dnd/node_modules/rollup/dist/rollup.js:9396:30)
at Module.error (/Users/nathan_power/workspaces/personal/react-beautiful-dnd/node_modules/rollup/dist/rollup.js:13333:9)
at handleMissingExport (/Users/nathan_power/workspaces/personal/react-beautiful-dnd/node_modules/rollup/dist/rollup.js:13255:21)
at Module.traceVariable (/Users/nathan_power/workspaces/personal/react-beautiful-dnd/node_modules/rollup/dist/rollup.js:13647:17)
at ModuleScope.findVariable (/Users/nathan_power/workspaces/personal/react-beautiful-dnd/node_modules/rollup/dist/rollup.js:12354:39)
at FunctionScope.findVariable (/Users/nathan_power/workspaces/personal/react-beautiful-dnd/node_modules/rollup/dist/rollup.js:2963:38)
at ChildScope.findVariable (/Users/nathan_power/workspaces/personal/react-beautiful-dnd/node_modules/rollup/dist/rollup.js:2963:38)
at FunctionScope.findVariable (/Users/nathan_power/workspaces/personal/react-beautiful-dnd/node_modules/rollup/dist/rollup.js:2963:38)
at ChildScope.findVariable (/Users/nathan_power/workspaces/personal/react-beautiful-dnd/node_modules/rollup/dist/rollup.js:2963:38)
at BlockScope.findVariable (/Users/nathan_power/workspaces/personal/react-beautiful-dnd/node_modules/rollup/dist/rollup.js:2963:38)

error Command failed with exit code 1.

Just for the record, I did a bit more testing and scroll-via-drag does raise the same error in IE11 for both keyboard and mouse drags in 12.0.0-alpha.4 (only tested mouse earlier).

@alexreardon
Copy link
Collaborator

The virtual branch is a bit messy

The window scroll logic is now handled in scroll-listener.js

@alexreardon
Copy link
Collaborator

It is okay if the branch doesn't pass for the PR

@alexreardon
Copy link
Collaborator

Most of the tests are broken :p

@nathanpower
Copy link
Author

I think all it needs is #1384, but I haven't been able to manually test, and unsure how to unit test now that the getWindow wrapper is gone.

Any chance you would be able include it in an alpha release, and I'll test it?

@alexreardon
Copy link
Collaborator

I will do one soon @nathanpower

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants