Skip to content

Commit

Permalink
WIP: First changes - checking whether returning null for index beyo…
Browse files Browse the repository at this point in the history
…nd a table boundaries will work properly #7031
  • Loading branch information
wszymanski committed Jul 8, 2020
1 parent aa83b7c commit e2eab29
Show file tree
Hide file tree
Showing 4 changed files with 23 additions and 8 deletions.
20 changes: 17 additions & 3 deletions src/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,13 @@ export default function Core(rootElement, userSettings, rootInstanceSymbol = fal
this.runHooks('afterSelection',
from.row, from.col, to.row, to.col, preventScrolling, selectionLayerLevel);
this.runHooks('afterSelectionByProp',
from.row, instance.colToProp(from.col), to.row, instance.colToProp(to.col), preventScrolling, selectionLayerLevel); // eslint-disable-line max-len
from.row,
from.col >= 0 ? instance.colToProp(from.col) : from.col, // We may start from the header
to.row,
instance.colToProp(to.col),
preventScrolling,
selectionLayerLevel
);

const isSelectedByAnyHeader = this.selection.isSelectedByAnyHeader();
const currentSelectedRange = this.selection.selectedRange.current();
Expand Down Expand Up @@ -283,7 +289,12 @@ export default function Core(rootElement, userSettings, rootInstanceSymbol = fal
this.runHooks('afterSelectionEnd',
from.row, from.col, to.row, to.col, selectionLayerLevel);
this.runHooks('afterSelectionEndByProp',
from.row, instance.colToProp(from.col), to.row, instance.colToProp(to.col), selectionLayerLevel);
from.row,
from.col >= 0 ? instance.colToProp(from.col) : from.col, // We may start from the header
to.row,
instance.colToProp(to.col),
selectionLayerLevel
);
});

this.selection.addLocalHook('afterIsMultipleSelection', (isMultiple) => {
Expand Down Expand Up @@ -1333,6 +1344,8 @@ export default function Core(rootElement, userSettings, rootInstanceSymbol = fal
]);
}

console.log(JSON.parse(JSON.stringify(changes)));

if (!changeSource && typeof row === 'object') {
changeSource = column;
}
Expand Down Expand Up @@ -2707,7 +2720,8 @@ export default function Core(rootElement, userSettings, rootInstanceSymbol = fal
physicalColumn = column;
}

const prop = datamap.colToProp(column);
const colToPropResult = datamap.colToProp(column);
const prop = colToPropResult !== null ? colToPropResult : column; // We can also get meta for columns beyond the table boundaries
const cellProperties = metaManager.getCellMeta(physicalRow, physicalColumn);

// TODO(perf): Add assigning this props and executing below code only once per table render cycle.
Expand Down
4 changes: 2 additions & 2 deletions src/dataMap.js
Original file line number Diff line number Diff line change
Expand Up @@ -202,9 +202,9 @@ class DataMap {

const physicalColumn = this.instance.toPhysicalColumn(column);

// Out of range, not visible column index.
// Beyond the table boundaries. // TODO: This conditional may be temporary.
if (physicalColumn === null) {
return column;
return null;
}

// Cached property.
Expand Down
3 changes: 2 additions & 1 deletion src/dataSource.js
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,8 @@ class DataSource {
const rangeEnd = this.countFirstRowKeys() - 1;

rangeEach(rangeStart, rangeEnd, (column) => {
const prop = this.colToProp(column);
const colToPropResult = this.colToProp(column);
const prop = colToPropResult !== null ? colToPropResult : column;

if (column >= (startColumn || rangeStart) && column <= (endColumn || rangeEnd) && !Number.isInteger(prop)) {
const cellValue = this.getAtPhysicalCell(row, prop, dataRow);
Expand Down
4 changes: 2 additions & 2 deletions test/e2e/core/colToProp.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ describe('Core.colToProp', () => {
hot.columnIndexMapper.setIndexesSequence([4, 3, 2, 1, 0]);

expect(colToProp(0)).toBe(4);
expect(colToProp(10)).toBe(10); // I'm not sure if this should return result like that by design.
expect(colToProp(10)).toBe(null);
});

it('should return proper value after calling the function when columns was reorganized (data is array of objects)', () => {
Expand All @@ -66,6 +66,6 @@ describe('Core.colToProp', () => {
hot.columnIndexMapper.setIndexesSequence([3, 2, 1, 0]);

expect(colToProp(0)).toBe('date');
expect(propToCol(10)).toBe(10); // I'm not sure if this should return result like that by design.
expect(colToProp(10)).toBe(null);
});
});

0 comments on commit e2eab29

Please sign in to comment.