Master no shared borders rar #5452
Conversation
|
|
rrahir
force-pushed
the
master-no-shared-borders-rar
branch
7 times, most recently
from
80de593 to
2e5bc12
Compare
src/plugins/core/borders.ts
Outdated
| group[group.length - 1] + 1, | ||
| group[0] | ||
| ); | ||
| const start = Math.min(group[0], group[group.length - 1]); |
There was a problem hiding this comment.
Shouldn't we use this start value at line 117 instead of group[0] + 1 ?
There was a problem hiding this comment.
(same comment for below)
There was a problem hiding this comment.
apparently I can't read, the elements were sorted in descending order on purpose on line 104 so there was no need to redefin start/end to begin with.
src/plugins/core/borders.ts
Outdated
| @@ -182,7 +143,7 @@ export class BordersPlugin extends CorePlugin<BordersPluginState> implements Bor | |||
| colLeftOfInsertion = cmd.base - 1; | |||
| colRightOfInsertion = cmd.base + cmd.quantity; | |||
| } else { | |||
| this.shiftBordersHorizontally(cmd.sheetId, cmd.base + 1, cmd.quantity, { | |||
| this.shiftBordersHorizontally(cmd.sheetId, cmd.base, cmd.quantity, { | |||
There was a problem hiding this comment.
Why did you change these lines but not the one above ? (l111 vs l146 and l117 vs l170) ?
There was a problem hiding this comment.
short: the tests pass so I didn't need to adapt it.
Long: it's been 2 hours and stillcan't figure out everything that happns. removing the +1 sees to have no impact, still on it
There was a problem hiding this comment.
So the functions 'shiftBorderXlly' seems to do some "scabreux" stuff depending on wheter or not we would provide a certain parameter, I guess this was kind of necessary when we had the previous data structure but now it doesn't seem required anymore. changed the behaviour so that the function is more strzaightforward on its purpose: shift all headers from a starting point from a given delta. Hence, the "before" insertion should act on the base header of the command (no +1) and the "after" insertion should only apply on the header after the insertion (hence the +1)
Currently, the borders are defined as a "vertical" and "horizontal" property on each cell and are shared through adjacent cells. E.g. The bottom border defined on `A1` is functionally also the top border of `A2` (see commit 9747294) for more info) This approach is not shared with other spreadsheet engines,be it Excel or GSheet. In their case, each cell has their own independant border and follows a simple rule: - setting the border of a cell will erase the border of the adjacent cell IIF the borders differ, i.e. Have a different color and/or style. E.g. - On A2, add an "all" blue border - on A1, add a bottom blue border - on A3, add a top red border By inserting rows between rows 1-2 and rows 2-3, you can see that A2 still has a top border but it no longer has a bottom border. This revision changes the internal structure of the border plugin to align our behaviour with the other spreadsheet engines. Task: 4458245
rrahir
force-pushed
the
master-no-shared-borders-rar
branch
from
2e5bc12 to
4cce6d5
Compare
Description:
description of this task, what is implemented and why it is implemented that way.
Task: TASK_ID
review checklist