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

Undoing changes will scroll us back to the changed area in specific case #10639

Merged
merged 11 commits into from Jan 5, 2024
8 changes: 8 additions & 0 deletions .changelogs/10639.json
@@ -0,0 +1,8 @@
{
"issuesOrigin": "private",
"title": "Undoing changes will scroll us back to the changed area in specific case",
"type": "added",
"issueOrPR": 10639,
"breaking": false,
"framework": "none"
}
77 changes: 77 additions & 0 deletions handsontable/src/plugins/undoRedo/__tests__/UndoRedo.spec.js
Expand Up @@ -3290,4 +3290,81 @@ describe('UndoRedo', () => {
expect(hot.getSettings().fixedColumnsStart).toBe(1);
});
});

describe('scroll', () => {
it('should move to the already changed cell only vertically', async() => {
const hot = handsontable({
data: Handsontable.helper.createSpreadsheetData(50, 50),
rowHeaders: true,
colHeaders: true,
width: 500,
height: 400,
});

selectCell(4, 4);
setDataAtCell(4, 4, 'aaaa');
selectCell(5, 4);
scrollViewportTo({ row: 25, col: 4, verticalSnap: 'top' });
undo();

expect(hot.view.getFirstFullyVisibleRow()).toBe(4);
expect(hot.view.getFirstFullyVisibleColumn()).toBe(0);
});

it('should move to the already changed cell only horizontally', async() => {
const hot = handsontable({
data: Handsontable.helper.createSpreadsheetData(50, 50),
rowHeaders: true,
colHeaders: true,
width: 500,
height: 400,
});

selectCell(4, 4);
setDataAtCell(4, 4, 'aaaa');
selectCell(5, 4);
scrollViewportTo({ row: 4, col: 25, horizontalSnap: 'start' });
undo();

expect(hot.view.getFirstFullyVisibleRow()).toBe(0);
expect(hot.view.getFirstFullyVisibleColumn()).toBe(4);
});

it('should move to the already changed cell on both axis', async() => {
const hot = handsontable({
data: Handsontable.helper.createSpreadsheetData(50, 50),
rowHeaders: true,
colHeaders: true,
width: 500,
height: 400,
});

selectCell(4, 4);
setDataAtCell(4, 4, 'aaaa');
selectCell(5, 4);
scrollViewportTo({ row: 25, col: 25 });
undo();

expect(hot.view.getFirstFullyVisibleRow()).toBe(4);
expect(hot.view.getFirstFullyVisibleColumn()).toBe(4);
});

it('should not move to the already changed cell when selection has not been changed', async() => {
const hot = handsontable({
data: Handsontable.helper.createSpreadsheetData(50, 50),
rowHeaders: true,
colHeaders: true,
width: 500,
height: 400,
});

selectCell(4, 4);
setDataAtCell(4, 4, 'aaaa');
scrollViewportTo({ row: 25, col: 25, horizontalSnap: 'start', verticalSnap: 'top' });
undo();

expect(hot.view.getFirstFullyVisibleRow()).toBe(25);
expect(hot.view.getFirstFullyVisibleColumn()).toBe(25);
});
});
});
31 changes: 31 additions & 0 deletions handsontable/src/plugins/undoRedo/undoRedo.js
Expand Up @@ -489,6 +489,37 @@ UndoRedo.ChangeAction.prototype.undo = function(instance, undoneCallback) {
}
}

const selectedLast = instance.getSelectedLast();

if (selectedLast !== undefined) {
const [changedRow, changedColumn] = data[0];
const [selectedRow, selectedColumn] = selectedLast;
const firstFullyVisibleRow = instance.view.getFirstFullyVisibleRow();
const firstFullyVisibleColumn = instance.view.getFirstFullyVisibleColumn();
const isInVerticalViewPort = changedRow >= firstFullyVisibleRow;
const isInHorizontalViewPort = changedColumn >= firstFullyVisibleColumn;
const isInViewport = isInVerticalViewPort && isInHorizontalViewPort;
const isChangedSelection = selectedRow !== changedRow || selectedColumn !== changedColumn;

// Performing scroll only when selection has been changed right after editing a cell.
if (isInViewport === false && isChangedSelection === true) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about the isChangedSelection condition.
It is in line with this comment, but does not meet the requirements of the original post of https://github.com/handsontable/dev-handsontable/issues/115 (it does not require selecting a different cell before hitting cmd+z)

Is this what we want to do with it?
cc: @evanSe

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jansiegel the current implementation in line with the comment is correct, @wszymanski we want a code sandbox for @krzysztofspilka review before this is merged to develop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jansiegel the current implementation in line with the comment is correct, @wszymanski we want a code sandbox for @krzysztofspilka review before this is merged to develop.

Here you go: https://jsfiddle.net/916z0tvc

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wszymanski we are good to merge

const scrollConfig = {
row: changedRow,
col: changedColumn,
};

if (isInVerticalViewPort === false) {
scrollConfig.verticalSnap = 'top';
}

if (isInHorizontalViewPort === false) {
scrollConfig.horizontalSnap = 'start';
}

instance.scrollViewportTo(scrollConfig);
}
}

instance.selectCells(this.selected, false, false);
};
UndoRedo.ChangeAction.prototype.redo = function(instance, onFinishCallback) {
Expand Down