Enable header row display in tables#241
Conversation
rossjones
force-pushed
the
table-header-display
branch
from
dd56a89 to
4035cf8
Compare
A quick outline of how the header rows can be optionally shown whenever a table is shown.
rossjones
force-pushed
the
table-header-display
branch
from
4035cf8 to
1107390
Compare
rossjones
changed the title
| for (var cell of selection.rightCells) { cell.node.classList.add(CellSelectedRightClassName); } | ||
| selection.focus.node.scrollIntoViewIfNeeded(false); | ||
|
|
||
| const headerRowCellCount = document.querySelectorAll('th[role="col"]').length |
There was a problem hiding this comment.
It's possible for there to be more than one table per page. So we should use table.querySelectorAll to scope it to just the table in question.
| selection.focus.node.scrollIntoViewIfNeeded(false); | ||
|
|
||
| const headerRowCellCount = document.querySelectorAll('th[role="col"]').length | ||
| const headerRowOffset = headerRowCellCount > 0 ? 1 : 0; |
There was a problem hiding this comment.
Would it make sense to set this to the number of header rows instead of just assuming there is only one? I ask because in other contexts we have used multiple header rows to contain e.g. per-column guidance, and its conceivable that we might do that again.
There was a problem hiding this comment.
What is the expected behaviour when a headerRange spans multiple rows? Presumably we don't know whether it is guidance or an extension of the header row text, or something else. I've fixed that use-case with the row count, but it isn't handled very elegantly elsewhere (e.g. we don't do anything with subsequent rows)
| for (var i = range[0]; i <= range[1]; i++) { | ||
| headers.push(base26.toBase26(i + 1)) | ||
| } |
There was a problem hiding this comment.
I think this is where we want to be using the index of the source column rather than assuming that this just starts from zero?
There was a problem hiding this comment.
This isn't iterating from 0, it's the first element in range, a two-element array with the start position and the end position. It's calculated in calculateHeaderRange . I've updated the range to be an object so that the iterations are for (var i = range.start; i <= range.end; i++) { instead.
| if (!Object.prototype.hasOwnProperty.call(session.headerRange, "start") || | ||
| !Object.prototype.hasOwnProperty.call(session.headerRange, "end")) { | ||
| console.warn("HeaderRowDisplay: No header range available for source headers") | ||
| return null | ||
| } |
There was a problem hiding this comment.
What will the headerRange be if the user skips the selection step? And/or, what if there is no selection step in this journey? See also my comment in the ticket about what we should do there.
| // With no specified headers, try and default to the first row. | ||
| const r = sheets_lib.GetRows(session) | ||
| if (r) { | ||
| return [0, r[0].length] | ||
| } |
There was a problem hiding this comment.
This feels like it is really a default header selection. Should we be handling that elsewhere?
There was a problem hiding this comment.
I think the default header selection is the entirety of the first row (where the length is constrained by the library to cells with data). This is the same logic, but we should probably formalise it somewhere
There was a problem hiding this comment.
Yes, I think I was imagining making it so that headerRange was always a valid range, i.e. just the first row or null row but with "column A-Z" type label,s so that this function just collapsed down to line 180.
There was a problem hiding this comment.
Checked this, and the default header row is selected if the user submits at "select a header" without a selection. This is just defensive and should probably belong in the backend rather than the frontend, but will move that in next refactor.
There was a problem hiding this comment.
Yes, I think I was imagining making it so that
headerRangewas always a valid range, i.e. just the first row or null row but with "column A-Z" type label,s so that this function just collapsed down to line 180.
Ah for some reason this comment didn't show up. Yeah, I think the null row makes the most sense as we don't want to add random text as column headings if the user doesn't choose anything. Will have to see where that breaks other logic and see if we can force the headers to have a row index that doesn't exist.
2 participants
Allows tables to have a table header row (THEAD) that contains values from the following list:
There are obvious contraints on their use (e.g. can't use target mode until mapping done, can't use source until a sheet is selected with a header range).