In the previous tutorial, we make columns swappable. I mentioned that in some cases two columns can swap really fast and flash. There is a flaw in our code. In this tutorial, we are going to fix it.
What
First of all, when we spot a bug, we need to identify what the bug is and try limiting the factors.
This one is working as expected.
This one flashes.
So what's the bug? When we move "website" column to the right, we expect it to swap with "company" column. The "website" column does swap with "company" column, but they somehow swap again and again when we keep moving towards right.
Let's try another column. This works as expected. When moving "company" column to left or right, "company" column only swap once with the "address" column .
It's not obvious where the bug occurs, but we know that "website" column has bug and "company" column does not. We can inspect on them further more. We move our cursor much slower this time. We can see that sometimes the columns swap correctly, but as soon as we move our cursor again. Even we are still moving "website" towards right, "website" column swaps back with its left column "company", which is not correct.
So the bug occurs when we move a column to right, it should only swap with its right column, but it sometimes swaps with its left column. When we constantly moving our cursor, the columns swap repeatedly and fast. That's why they flash.
We have identified the bug. Next, we want to collect as much information as possible. Does this bug also occurs when we move a column to left? Let's test it out.
Moving "company" column to left works fine.
Moving "website" column to left has bugs.
Why
After we identified the bug, we want to figure out why it occurs. We tested "company" column and "website" column. The bug only occurs when we move "website" column but not "company" column. What's the difference between them? The only difference I can tell is their sizes. The "company" column is much wider than the "website" column, but this information doesn't seem to be helpful. So we look for next clue. How do we swap the columns?
function makeColumnsSwappable(columnsContainer, elementsToPatch = []) {
columnsContainer.addEventListener('pointerdown', e => {
let columnElements = [...columnsContainer.children]
const firstTarget = e.target
let firstTargetIndex = columnElements.indexOf(firstTarget)
function preventDefault(e) {
e.preventDefault()
}
document.addEventListener('selectstart', preventDefault)
if (firstTargetIndex === -1)
return
function handleMove(e) {
const secondTarget = e.target
const secondTargetIndex = columnElements.indexOf(secondTarget)
if (secondTargetIndex === -1)
return
if (firstTarget === secondTarget)
return
// (2) this is where we want to swap columns
swapColumns(columnsContainer, firstTargetIndex, secondTargetIndex)
elementsToPatch.forEach((columnsContainer) => {
swapColumns(columnsContainer, firstTargetIndex, secondTargetIndex)
})
columnElements = [...columnsContainer.children]
firstTargetIndex = columnElements.indexOf(firstTarget)
}
function swapColumns(container, firstTargetIndex, secondTargetIndex) {
const columns = container.children
const firstTarget = columns[firstTargetIndex]
const secondTarget = columns[secondTargetIndex]
const isMoveToLeft = firstTargetIndex > secondTargetIndex
const isMoveToRight = firstTargetIndex < secondTargetIndex
// (3) this is where we actually swap the column elements
if (isMoveToLeft) {
secondTarget.insertAdjacentElement('beforebegin', firstTarget)
} else if (isMoveToRight) {
secondTarget.insertAdjacentElement('afterend', firstTarget)
}
}
// (1) this is where we handle 'pointermove' event
columnsContainer.addEventListener('pointermove', handleMove)
document.addEventListener('pointerup', () => {
columnsContainer.removeEventListener('pointermove', handleMove)
document.removeEventListener('selectstart', preventDefault)
}, { once: true })
})
}
We swap two column elements by getting two targets, the first target is the one on 'pointerdown', and the second target is the one on 'pointermove'. We assume that we are moving first column to left if firstTargetIndex > secondTargetIndex, and moving first column to right if firstTargetIndex < secondTargetIndex. We use that information to move elements with insertAdjacentElement.
function swapColumns(container, firstTargetIndex, secondTargetIndex) {
const columns = container.children
const firstTarget = columns[firstTargetIndex]
const secondTarget = columns[secondTargetIndex]
const isMoveToLeft = firstTargetIndex > secondTargetIndex
const isMoveToRight = firstTargetIndex < secondTargetIndex
if (isMoveToLeft) {
secondTarget.insertAdjacentElement('beforebegin', firstTarget)
} else if (isMoveToRight) {
secondTarget.insertAdjacentElement('afterend', firstTarget)
}
}
We can probably tell that using firstTargetIndex and secondTargetIndex to get moving direction is not ideal. But the logic works sometimes. So what goes wrong? Let's look closely to the cases that works and that doesn't this time.
When we are moving "company" column, it looks like the cursor is always on top of it.
When we are moving "website" column, as soon as it swap with "company" column, the cursor is on top of the "company" column.
In our swapping columns logic, we do nothing if first target and second target are the same, and we swap two columns if first target and second target are different. This reflects in the result. The reason that swapping "website" column with "company" column has bug is because "company" column is much wider than "website" column. After "website" column swaps with "company" column, the cursor becomes on top of "company" column. As soon as we move cursor again, they swap. After they swap, the cursor is still on top of "company" column. This repeats again and again on 'pointermove'. That's why they flash.
How
After we find out the reason of the bug, we can start thinking how to fix it. If you'd like to follow along, here is the code from the previous tutorial.
When we are moving a column to right, we only want it to swap with the column on its right. To do that, we need to check if cursor is moving towards right. The same logic applies to moving a column to left.
function makeColumnsSwappable(columnsContainer, elementsToPatch = []) {
columnsContainer.addEventListener('pointerdown', e => {
// save first cursor x position on 'pointerdown'
// e.clientX is the horizontal distance
// from the left edge of browser window
// to where the event is triggered
let lastCursorX = e.clientX
let columnElements = [...columnsContainer.children]
const firstTarget = e.target
let firstTargetIndex = columnElements.indexOf(firstTarget)
function preventDefault(e) {
e.preventDefault()
}
document.addEventListener('selectstart', preventDefault)
if (firstTargetIndex === -1)
return
function handleMove(e) {
// save new cursor x position on 'pointermove'
const newCursorX = e.clientX
const secondTarget = e.target
const secondTargetIndex = columnElements.indexOf(secondTarget)
if (secondTargetIndex === -1)
return
if (firstTarget === secondTarget)
return
// compare `newCursorX` and `lastCursorX` to check
// which direction the cursor is moving towards
const isMoveToLeft = newCursorX < lastCursorX
const isMoveToRight = newCursorX > lastCursorX
// update `lastCursorX` for next comparison
lastCursorX = newCursorX
swapColumns(columnsContainer, firstTargetIndex, secondTargetIndex)
elementsToPatch.forEach((columnsContainer) => {
swapColumns(columnsContainer, firstTargetIndex, secondTargetIndex)
})
columnElements = [...columnsContainer.children]
firstTargetIndex = columnElements.indexOf(firstTarget)
}
function swapColumns(container, firstTargetIndex, secondTargetIndex) {
const columns = container.children
const firstTarget = columns[firstTargetIndex]
const secondTarget = columns[secondTargetIndex]
const isMoveToLeft = firstTargetIndex > secondTargetIndex
const isMoveToRight = firstTargetIndex < secondTargetIndex
if (isMoveToLeft) {
secondTarget.insertAdjacentElement('beforebegin', firstTarget)
} else if (isMoveToRight) {
secondTarget.insertAdjacentElement('afterend', firstTarget)
}
}
columnsContainer.addEventListener('pointermove', handleMove)
document.addEventListener('pointerup', () => {
columnsContainer.removeEventListener('pointermove', handleMove)
document.removeEventListener('selectstart', preventDefault)
}, { once: true })
})
}
Update logic with new isMoveToLeft and isMoveToRight
function handleMove(e) {
//...
// pass `isMoveToLeft` and `isMoveToRight`
swapColumns(columnsContainer, firstTargetIndex, secondTargetIndex, isMoveToLeft, isMoveToRight)
elementsToPatch.forEach((columnsContainer) => {
// pass `isMoveToLeft` and `isMoveToRight`
swapColumns(columnsContainer, firstTargetIndex, secondTargetIndex, isMoveToLeft, isMoveToRight)
})
// ...
}
// update to take two more parameter
// `isMoveToLeft` and `isMoveToRight`
function swapColumns(container, firstTargetIndex, secondTargetIndex, isMoveToLeft, isMoveToRight) {
const columns = container.children
const firstTarget = columns[firstTargetIndex]
const secondTarget = columns[secondTargetIndex]
// no longer using `firstTargetIndex` and `secondTargetIndex`
// to calculate `isMoveToLeft` and `isMoveToRight` from here
if (isMoveToLeft) {
secondTarget.insertAdjacentElement('beforebegin', firstTarget)
} else if (isMoveToRight) {
secondTarget.insertAdjacentElement('afterend', firstTarget)
}
}
Great! By making the logic correct, the bug is fixed!
One last thing to do is to refactor our code a little bit. function swapColumns now takes 5 parameters in order, if we accidently pass the arguments in a wrong order, the function won't work. The parameters are all required, so what can we do? Well, we can use object to make those parameters as a group, so the order won't matter.
// `swapColumns` now only have two parameters
// the second one is an object
function swapColumns(container, {
firstTargetIndex,
secondTargetIndex,
isMoveToLeft,
isMoveToRight
}) { //...
Group the arguments to an object
function handleMove(e) {
//...
swapColumns(columnsContainer, {
// as long as we provide these in this object
// the order of them doesn't matter at all
firstTargetIndex,
secondTargetIndex,
isMoveToLeft,
isMoveToRight
})
elementsToPatch.forEach((columnsContainer) => {
swapColumns(columnsContainer, {
// the order is different than above
isMoveToLeft,
isMoveToRight,
secondTargetIndex,
firstTargetIndex
})
})
// ...
}
Since the two objects we pass to swapColumns have the exact same properties, We can also group it like this.
const swapColumnInfo = {
firstTargetIndex,
secondTargetIndex,
isMoveToLeft,
isMoveToRight
}
swapColumns(columnsContainer, swapColumnInfo)
elementsToPatch.forEach((columnsContainer) => {
swapColumns(columnsContainer, swapColumnInfo)
})
The code should still work. Note that grouping all swapColumns parameters to one object is also possible. It will require slightly more change. You can try it out as an exercise. Just to be clear, this refactor is not necessary as we probably won't use swapColumns elsewhere. I merely want to show that how to modify parameters to make a function easier to use.
Top comments (0)