Render only needed sortable items #1096
Conversation
…eated and extra render
packages/core/src/store/actions.ts
Outdated
| @@ -2,10 +2,9 @@ import type {Coordinates, UniqueIdentifier} from '../types'; | |||
| import type {DroppableContainer} from './types'; | |||
|
|
|||
| export enum Action { | |||
| DragStart = 'dragStart', | |||
| SetInitiailCoordinates = 'setInitialCoordinates', | |||
There was a problem hiding this comment.
| SetInitiailCoordinates = 'setInitialCoordinates', | |
| SetInitialCoordinates = 'setInitialCoordinates', |
small typo
There was a problem hiding this comment.
thanks, can't commit as is - need to rename all the usages.
I will do it later
`dnd-kit` has a problem where, when drag events start and stop, every item that uses the library rerenders. This occurs due to its use of context. The dnd library needs to listen for pointer events to handle dragging. Because our images are both clickable (selectable) and draggable, every time you click an image, the dnd necessarily sees this event, its context updates and all other dnd-enabled components rerender. With a lot of images in gallery and/or batch manager, this leads to some jank. There is an open PR to address this: clauderic/dnd-kit#1096 But unfortunately, the maintainer hasn't accepted any changes for a few months, and its not clear if this will be merged any time soon :/ This change simply extracts the draggable and droppable logic out of IAIDndImage into their own minimal components. Now only these need to rerender when the dnd context is changed. The rerenders are far less impactful now. Hopefully the linked PR is accepted and we get even more efficient dnd functionality in the future.
`dnd-kit` has a problem where, when drag events start and stop, every item that uses the library rerenders. This occurs due to its use of context. The dnd library needs to listen for pointer events to handle dragging. Because our images are both clickable (selectable) and draggable, every time you click an image, the dnd necessarily sees this event, its context updates and all other dnd-enabled components rerender. With a lot of images in gallery and/or batch manager, this leads to some jank. There is an open PR to address this: clauderic/dnd-kit#1096 But unfortunately, the maintainer hasn't accepted any changes for a few months, and its not clear if this will be merged any time soon :/ This change simply extracts the draggable and droppable logic out of IAIDndImage into their own minimal components. Now only these need to rerender when the dnd context is changed. The rerenders are far less impactful now. Hopefully the linked PR is accepted and we get even more efficient dnd functionality in the future. Also changed dnd activation constraint to distance (currently 10px) instead of delay and updated the stacking context of IAIDndImage subcomponents so that the reset and upload buttons still work.
`dnd-kit` has a problem where, when drag events start and stop, every item that uses the library rerenders. This occurs due to its use of context. The dnd library needs to listen for pointer events to handle dragging. Because our images are both clickable (selectable) and draggable, every time you click an image, the dnd necessarily sees this event, its context updates and all other dnd-enabled components rerender. With a lot of images in gallery and/or batch manager, this leads to some jank. There is an open PR to address this: clauderic/dnd-kit#1096 But unfortunately, the maintainer hasn't accepted any changes for a few months, and its not clear if this will be merged any time soon :/ This change simply extracts the draggable and droppable logic out of IAIDndImage into their own minimal components. Now only these need to rerender when the dnd context is changed. The rerenders are far less impactful now. Hopefully the linked PR is accepted and we get even more efficient dnd functionality in the future.
`dnd-kit` has a problem where, when drag events start and stop, every item that uses the library rerenders. This occurs due to its use of context. The dnd library needs to listen for pointer events to handle dragging. Because our images are both clickable (selectable) and draggable, every time you click an image, the dnd necessarily sees this event, its context updates and all other dnd-enabled components rerender. With a lot of images in gallery and/or batch manager, this leads to some jank. There is an open PR to address this: clauderic/dnd-kit#1096 But unfortunately, the maintainer hasn't accepted any changes for a few months, and its not clear if this will be merged any time soon :/ This change simply extracts the draggable and droppable logic out of IAIDndImage into their own minimal components. Now only these need to rerender when the dnd context is changed. The rerenders are far less impactful now. Hopefully the linked PR is accepted and we get even more efficient dnd functionality in the future. Also changed dnd activation constraint to distance (currently 10px) instead of delay and updated the stacking context of IAIDndImage subcomponents so that the reset and upload buttons still work.
`dnd-kit` has a problem where, when drag events start and stop, every item that uses the library rerenders. This occurs due to its use of context. The dnd library needs to listen for pointer events to handle dragging. Because our images are both clickable (selectable) and draggable, every time you click an image, the dnd necessarily sees this event, its context updates and all other dnd-enabled components rerender. With a lot of images in gallery and/or batch manager, this leads to some jank. There is an open PR to address this: clauderic/dnd-kit#1096 But unfortunately, the maintainer hasn't accepted any changes for a few months, and its not clear if this will be merged any time soon :/ This change simply extracts the draggable and droppable logic out of IAIDndImage into their own minimal components. Now only these need to rerender when the dnd context is changed. The rerenders are far less impactful now. Hopefully the linked PR is accepted and we get even more efficient dnd functionality in the future.
`dnd-kit` has a problem where, when drag events start and stop, every item that uses the library rerenders. This occurs due to its use of context. The dnd library needs to listen for pointer events to handle dragging. Because our images are both clickable (selectable) and draggable, every time you click an image, the dnd necessarily sees this event, its context updates and all other dnd-enabled components rerender. With a lot of images in gallery and/or batch manager, this leads to some jank. There is an open PR to address this: clauderic/dnd-kit#1096 But unfortunately, the maintainer hasn't accepted any changes for a few months, and its not clear if this will be merged any time soon :/ This change simply extracts the draggable and droppable logic out of IAIDndImage into their own minimal components. Now only these need to rerender when the dnd context is changed. The rerenders are far less impactful now. Hopefully the linked PR is accepted and we get even more efficient dnd functionality in the future. Also changed dnd activation constraint to distance (currently 10px) instead of delay and updated the stacking context of IAIDndImage subcomponents so that the reset and upload buttons still work.
`dnd-kit` has a problem where, when drag events start and stop, every item that uses the library rerenders. This occurs due to its use of context. The dnd library needs to listen for pointer events to handle dragging. Because our images are both clickable (selectable) and draggable, every time you click an image, the dnd necessarily sees this event, its context updates and all other dnd-enabled components rerender. With a lot of images in gallery and/or batch manager, this leads to some jank. There is an open PR to address this: clauderic/dnd-kit#1096 But unfortunately, the maintainer hasn't accepted any changes for a few months, and its not clear if this will be merged any time soon :/ This change simply extracts the draggable and droppable logic out of IAIDndImage into their own minimal components. Now only these need to rerender when the dnd context is changed. The rerenders are far less impactful now. Hopefully the linked PR is accepted and we get even more efficient dnd functionality in the future.
`dnd-kit` has a problem where, when drag events start and stop, every item that uses the library rerenders. This occurs due to its use of context. The dnd library needs to listen for pointer events to handle dragging. Because our images are both clickable (selectable) and draggable, every time you click an image, the dnd necessarily sees this event, its context updates and all other dnd-enabled components rerender. With a lot of images in gallery and/or batch manager, this leads to some jank. There is an open PR to address this: clauderic/dnd-kit#1096 But unfortunately, the maintainer hasn't accepted any changes for a few months, and its not clear if this will be merged any time soon :/ This change simply extracts the draggable and droppable logic out of IAIDndImage into their own minimal components. Now only these need to rerender when the dnd context is changed. The rerenders are far less impactful now. Hopefully the linked PR is accepted and we get even more efficient dnd functionality in the future. Also changed dnd activation constraint to distance (currently 10px) instead of delay and updated the stacking context of IAIDndImage subcomponents so that the reset and upload buttons still work.
|
@alissaVrk Can you publish this changes in npm as a custom modified dnd-kit package? |
|
@bryanjtc I'll try. |
Sure |
|
@hexwit The bug described above was a result of the typo, so adding items to the dependency array solves it. The only major issue I'm experiencing now is related to the initial rendering of conditionally rendered items - e.g., you have two selectable tabs A and B, each conditionally rendering hundreds of sortable items when clicked. If you click on B, for instance, the initial render of the sortable items associated with that tab usually takes a long time, frequently freezing the app. @alissaVrk You had mentioned the possibility of bringing further improvements to this already amazing contribution - any idea of what those could be or if you would be willing to implement some of them? |
|
@bryanjtc @tomasmenezes published this PR at @alissavrk/dnd-kit-sortable, @alissavrk/dnd-kit-core etc. there is a sandbox that uses these versions I did bump versions but didn't update the readme or anything else.. @tomasmenezes about further improvements, I don't remember exactly, one of them had to do with rendering all items on drop. but what is the point if it doesn't get merged? I can probably recreate the list of API changes I thought would be nice I guess we could create a new library based on this one, but I don't really understand how are people expected to find it :) |
|
any update on this? |
|
@clauderic I'd love to see this merged into a major version of dnd-kit. We're currently using it at my company and are having some rerendering issues related to |
|
@alissaVrk Sounds amazing! There is still much to improve. Imo continuing this work is extremely meaningful as it has a huge measurable impact on the community's experience - especially regarding essential performance issues such as this one, that should be quickly addressed. |
|
@alissaVrk Hello, can u help me? I use react + vite + ts and I got next error after npm i ur packages [plugin:vite:import-analysis] Failed to resolve entry for package "@alissavrk/dnd-kit-core". The package may have incorrect main/module/exports specified in its package.json. |
| @@ -12,7 +12,11 @@ interface Props { | |||
| } | |||
|
|
|||
| export function RestoreFocus({disabled}: Props) { | |||
| const {active, activatorEvent, draggableNodes} = useContext(InternalContext); | |||
| const {useGloablActive, useGlobalActivatorEvent, draggableNodes} = | |||
There was a problem hiding this comment.
Minor, just a typo here (should be useGlobalActive)
|
@kirill2400 hi, sorry that it took me so long.. |
I have a similar problem with your package, apparently something is missing inside the package to make it work correctly with vite as I understand it |
|
@alissaVrk @kirill2400 I've identified and "fixed" the issue with Vite. The packages built and published get built under the name To "fix" this, I have added the following yarn scripts to my project's "fix-dnd-kit-fork-accessibility": "json -I -f ./node_modules/@alissavrk/dnd-kit-accessibility/package.json -e \"this.module=\\\"dist/dnd-kit-accessibility.esm.js\\\"\"",
"fix-dnd-kit-fork-core": "json -I -f ./node_modules/@alissavrk/dnd-kit-core/package.json -e \"this.module=\\\"dist/dnd-kit-core.esm.js\\\"\"",
"fix-dnd-kit-fork-modifiers": "json -I -f ./node_modules/@alissavrk/dnd-kit-modifiers/package.json -e \"this.module=\\\"dist/dnd-kit-modifiers.esm.js\\\"\"",
"fix-dnd-kit-fork-sortable": "json -I -f ./node_modules/@alissavrk/dnd-kit-sortable/package.json -e \"this.module=\\\"dist/dnd-kit-sortable.esm.js\\\"\"",
"fix-dnd-kit-fork-utilities": "json -I -f ./node_modules/@alissavrk/dnd-kit-utilities/package.json -e \"this.module=\\\"dist/dnd-kit-utilities.esm.js\\\"\"",
"fix-dnd-kit-fork": "yarn fix-dnd-kit-fork-accessibility & yarn fix-dnd-kit-fork-core & yarn fix-dnd-kit-fork-modifiers & yarn fix-dnd-kit-fork-sortable & yarn fix-dnd-kit-fork-utilities"By running It's a hacky solution, but should do the trick until this gets oficially merged in, and you want to use this fork instead of the official dnd-kit release. Edit: these scripts assume you're using yarn and have the |
|
@alissaVrk, don't know how this will integrate with the new planned API but would you still be able to generate that list of API changes you had in mind to take these performance improvements further? |
|
@alissaVrk btw, that performance issue I mentioned earlier in the thread is exemplified in this stress test sandbox. Simply changing the active window takes removeChild for a ride. The instance usually crashes when the console is active, and it doesn't seem like further memoization does much. |
|
@tomasmenezes hi, sorry it took me so long (I'm not using this lib anywhere, and back to work :) ). so once it's production-ready things should be better |
|
hi @alissaVrk, @tomasmenezes! |
|
@alissaVrk Regardless, thank you for all of your insights! They are certainly helping to push the library in the right direction! |
|
@clauderic, are there any reasons to not merge this PR (once conflicts are resolved) and release a new versions as an immediate fix for performance related issues that's been bugging the library for a while now? I mean - assuming that the experimental branch production release date is probably not anywhere close (as it's barely an alpha), wouldn't that make sense to fix the issues for all the current users of the library? 🤔 |
fixes #1071, fixes #994, fixes #898 and probably fixes #943 and others
based on #1088 so all changes that were there are here as well
still re-renders all the sortable items if the passed items into context change. probably fixable as well, but I think this is already much better.
it was impossible to do without changes to the API and implementation:
getNewIndexis now passed to the context instead ofuseSortablelocalStrategy, I couldn't find a use case that can't be solved in global strategy (I'm passing id there as well) if you know of one let me knowuseDroppableactive is defined only if it's over the itemuseSortabledoes not exposeisSorting'activeIndexandoverIndex`useSortableactive is defined only for active and over itemsuseSortableover is defined only for active and over itemsall stories seem to work as before with a few changes.
added tests to verify which items are re-rendered
** I think that
activeandovernames should be changed to explain if I am over some item or some item is over meand
strategyandgetNewIndexshould be connected, alsoanimateLayoutChangesis very confusing