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

Improve TableWalker yielded options. #6785

Closed
jodator opened this issue May 11, 2020 · 3 comments · Fixed by #6834
Closed

Improve TableWalker yielded options. #6785

jodator opened this issue May 11, 2020 · 3 comments · Fixed by #6834
Assignees
Labels
bc:minor Resolving this issue will introduce a minor breaking change. package:table type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@jodator
Copy link
Contributor

jodator commented May 11, 2020

📝 Provide a description of the improvement

TableWalker - intro

The purpose of the table walker is to iterate over a table "slots" not only table cells. The latter is a trivial task.

Iterating over table "slots" is required to handle operations on cells covering many "slots" (ie table cell with rowspan and/or colspan attributes).

TableWalker is used in two "modes":

  1. Direct iterator - extracting some information about from a table without modifying it.
  2. Constructing "tableMap" to modify a table - in most cases using slot information (and "previous cell").

The idea of "slots" is defined in W3C/WHATWG docs about tables and it makes sense to use that lingo.

Current TableWalkerValue

const tablewWalkerValue = {
    row: 1, // Often used
    column: 2,  // Often used
    cell: TableCell( { colspan: 2, rowspan: 2 } ),  // Often used
    colspan: 2, // Often used
    rowspan: 2, // Often used
    isSpanned: true, // <- confusing
    cellIndex: 0 // rarely used - most often previousCell is needed (batch insertions)
}

The most confusing here is isSpanned and cell. For slots that are covered by a table cell anchored in another slot we set isSpanned = true and the cell holds reference to the cell that spans over given slot.


Proposal

The language used in TableWalkerValue is hard to grasp. The better idea is to use "slot" definition form processing table model specification.

Introduce TableSlot typedef

The TableSlot is a name that is in line with docs describing table model - it describes over what we actually iterate.

const tableSlot = {
    // Current slot indexes
    row: 1, // starts from 1
    column: 2,
    // Is the slot also an anchor? Better name needed.
    isAnchorCell: false, // True if cell is anchored in current slot.
    // Anchor cell - a cell that covers this slot.
    anchorCell: TableCell( { colspan: 2, rowspan: 2 } ),
    // Indexes of anchor cell - calculating this outside is expensive.
    anchorCellRow: 0,
    anchorCellColumn: 0,
    // Getter to make it read when needed becasue we do not store span=1 in the model.
    anchorCellWidth: () => { this.anchorCell.getAttribute( 'colspan' ) || 1 },
    // Better than `colspan` & `rowspan`.
    anchorCellHeight: () => { this.anchorCell.getAttribute( 'rowpsan' ) || 1 }
}

However, we could drop anchor here to have a bit more compact names:

const tableSlot = {
    row: 1,
    column: 2,
    isAnchorCell: false,
    cell: TableCell( { colspan: 2, rowspan: 2 } ),
    cellRow: 0,
    cellColumn: 0,
    cellWidth: () => { this.cell.getAttribute( 'colspan' ) || 1 },
    cellHeight: () => { this.cell.getAttribute( 'rowpsan' ) || 1 }
}

Creating a table walker - restricting yield slots

In most cases we do not need whole table information. Some algorithms operate on rows while others on columns. However, some will operate on a table section constrained by rows and columns.

new TableWalker( table, {
    onlyAnchorCells: true, // (default value - which is kinda messy - )
    // hideNonAnchorSlots: false // (better default value, worse name)
    // Limit rows to return.
    startRow: 0, // (default value)
    endRow: tableHeight - 1, // (default value)
    // Limit columns to return.
    startColumn: 0, // (default value)
    endColumn: tableWidth - 1, // (default value)
    // was: column - iterate over one column only - I'd drop that.
} )

Modifying table problem

Moved to: #7326


If you'd like to see this improvement implemented, add a 👍 reaction to this post.

@jodator jodator added type:improvement This issue reports a possible enhancement of an existing feature. package:table labels May 11, 2020
@jodator jodator added this to the backlog milestone May 11, 2020
@jodator jodator added bc:minor Resolving this issue will introduce a minor breaking change. and removed bc:minor Resolving this issue will introduce a minor breaking change. labels May 11, 2020
@jodator jodator changed the title Table walker should handle startColumn and endColumn options. Improve Table walker yielded options. May 13, 2020
@jodator
Copy link
Contributor Author

jodator commented May 13, 2020

Another usefull information is to get anchor column / row for spanned cell.

@jodator jodator changed the title Improve Table walker yielded options. Improve TableWalker yielded options. May 15, 2020
@niegowski niegowski self-assigned this May 15, 2020
@niegowski niegowski modified the milestones: backlog, iteration 32 May 22, 2020
@jodator
Copy link
Contributor Author

jodator commented May 26, 2020

Issue to handle when fixing this: #3275.

@jodator
Copy link
Contributor Author

jodator commented May 28, 2020

After F2F talk we agreed on TableSlot class (because of getters):

const tableSlot = {
    row: 1,
    column: 2,
    // Below is OK. Also might be a getter.
    isAnchor: () => { return this.row === this.anchorRow && ... },
    cell: TableCell( { colspan: 2, rowspan: 2 } ),
    cellAnchorRow: 0, // More fluent.
    cellAnchorColumn: 0, // More fluent.
    // Below are OK as in line with HTML table specs.
    cellWidth: () => { this.cell.getAttribute( 'colspan' ) || 1 }, 
    cellHeight: () => { this.cell.getAttribute( 'rowpsan' ) || 1 },
    cellIndex // Keep it as is for now (or try to remove if no big work involved)
}

As for TableWalker options - we see that row and column are useful. We should make them clear in the docs what is the precedence and shouldn't be mixed with startRow / endRow or will just simply override startRow / endRow. (for row, similar for column).

new TableWalker( table, {
    includeAllSlots: false, // (default value)
    startRow: 0, // (default value)
    endRow: tableHeight - 1, // (default value)
    startColumn: 0, // (default value)
    endColumn: tableWidth - 1, // (default value)
} );

// OR

new TableWalker( table, {
    includeAllSlots: true,
    row: 0, // (default value is undefined)
    column: 0, // (default value is undefined)
} );

@jodator jodator added the bc:minor Resolving this issue will introduce a minor breaking change. label May 29, 2020
jodator added a commit that referenced this issue Jun 1, 2020
Other (table): Refactor values returned by the `TableWalker` iterator. Closes #6785.

Other (table): Add `row`, `startColumn`, and `endColumn` options to `TableWalker` constructor. See #6785.

MINOR BREAKING CHANGE (table): The values returned by the `TableWalker` iterator have changed. See #6785.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bc:minor Resolving this issue will introduce a minor breaking change. package:table type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants