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.

Sign up for GitHub

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: add and remove focused-cell part when scrolling grid #7659

Closed
wants to merge 6 commits into from
Closed

fix: add and remove focused-cell part when scrolling grid #7659

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4b1be2f
add/remove focused-cell
FrediWa Aug 16, 2024
cbf2ff8
refactor: remove redundant predicate
FrediWa Aug 16, 2024
9511459
refactor: utilize updatePart
FrediWa Aug 19, 2024
262f131
Merge branch 'main' into fix/focused-cell-while-scrolling
FrediWa Aug 28, 2024
b29d9fe
test: update scroll test
FrediWa Aug 28, 2024
835e6c1
use double underscore, remove useless let and use index instead of vi…
FrediWa Aug 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions packages/grid/src/vaadin-grid-mixin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import { DragAndDropMixin } from './vaadin-grid-drag-and-drop-mixin.js';
import { DynamicColumnsMixin } from './vaadin-grid-dynamic-columns-mixin.js';
import { EventContextMixin } from './vaadin-grid-event-context-mixin.js';
import { FilterMixin } from './vaadin-grid-filter-mixin.js';
import { updatePart } from './vaadin-grid-helpers.js';
import {
getBodyRowCells,
iterateChildren,
Expand Down Expand Up @@ -855,6 +856,16 @@ export const GridMixin = (superClass) =>
this._getItem(index, row);
}

/** @private */
_updateFocusedItem(row) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
_updateFocusedItem(row) {
__updateFocusedItem(row) {

We wanted to start using double underscores for private methods.

Copy link
Member

Choose a reason for hiding this comment

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

https://polymer-library.polymer-project.org/3.0/docs/devguide/properties#private-and-protected-properties

const focusedItemInRow = row.__virtualIndex === this._focusedItemIndex;
let isFocusedCell;
iterateChildren(row, (cell) => {
isFocusedCell = cell === this._focusedCell;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this comparison is safe to use with the virtual scrolling mechanism. If you scroll down and up again, is the cell in the focused row guaranteed to reuse the same cell instance that was previously stored as _focusedCell?

Maybe @tomivirkki can provide some insight?

Copy link
Contributor

Choose a reason for hiding this comment

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

From some testing it seems that this is not the case:

Bildschirmaufnahme.2024-08-29.um.08.57.58.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How did you do that? And did you get focus just with a click? With 2 finger scrolling, it works for me as expected and I lose focus as soon as I press the scrollbar's handle

updatePart(cell, isFocusedCell && focusedItemInRow, 'focused-cell');
});
}

/** @private */
_columnTreeChanged(columnTree) {
this._renderColumnTree(columnTree);
Expand Down Expand Up @@ -957,6 +968,7 @@ export const GridMixin = (superClass) =>
_updateItem(row, item) {
row._item = item;
const model = this.__getRowModel(row);
this._updateFocusedItem(row);

this._toggleDetailsCell(row, model.detailsOpened);

Expand Down
14 changes: 14 additions & 0 deletions packages/grid/test/styling.common.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,5 +315,19 @@ describe('styling', () => {
expect(newHeaderCell.getAttribute('part')).to.contain('foobar');
expect(newFooterCell.getAttribute('part')).to.contain('bazqux');
});

it('should add/remove focused-cell part name when scrolling', async () => {
firstCell.focus();
await nextFrame();

expect(grid.$.items.querySelector('tr:not([hidden]) [part~="focused-cell"')).to.not.be.null;

// Scrolls a completely new chunk of items into view.
grid.scrollToIndex(50);
expect(grid.$.items.querySelector('tr:not([hidden]) [part~="focused-cell"')).to.be.null;

grid.scrollToIndex(0);
expect(grid.$.items.querySelector('tr:not([hidden]) [part~="focused-cell"')).to.not.be.null;
});
});
});
1 change: 1 addition & 0 deletions packages/grid/theme/lumo/vaadin-grid-styles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ registerStyles(
cursor: default;
--_cell-padding: var(--vaadin-grid-cell-padding, var(--_cell-default-padding));
--_cell-default-padding: var(--lumo-space-xs) var(--lumo-space-m);
outline: none;
}

[part~='cell'] ::slotted(vaadin-grid-cell-content) {
Expand Down
Loading