Table viz column header tooltip extended to add data quality stats #11520
Conversation
…sualization.enso Co-authored-by: Radosław Waśko <[email protected]>
…sualization.enso Co-authored-by: Gregory Michael Travis <[email protected]>
…//github.com/enso-org/enso into wip/mk/add-data-quality-indicators-null-vals
…ization.vue Co-authored-by: Kaz Wesley <[email protected]>
…//github.com/enso-org/enso into wip/mk/add-data-quality-indicators-null-vals
marthasharkey
requested review from
Frizi,
farmaazon,
vitvakatu,
kazcw and
AdRiley
as code owners
app/gui/src/project-view/components/visualizations/TableVisualization.vue
Outdated
Show resolved
Hide resolved
| @@ -391,7 +316,7 @@ function addRowIndex(data: object[]): object[] { | |||
| return data.map((row, i) => ({ [INDEX_FIELD_NAME]: i, ...row })) | |||
| } | |||
|
|
|||
| function toField(name: string, valueType?: ValueType | null | undefined): ColDef { | |||
| function toField(name: string, index?: number, valueType?: ValueType | null | undefined): ColDef { | |||
There was a problem hiding this comment.
Because this function has multiple optional parameters, passing an options object containing the optional values as fields could make its call sites easier to read.
The parameters could also use some documentation: It's not obvious when index is expected to be present or what happens if its absent.
| !(dataQuality.number_of_whitespace && dataQuality.number_of_whitespace[index]) | ||
| : true | ||
|
|
||
| const hideDataQuality = nothingIsZeroOrNull && whitespaceIsZeroOrNull |
There was a problem hiding this comment.
I like to stick to the JS convention that "value absent" is falsy and "value present" is truthy--in this case that would mean inverting the logic of this and its sub-computations, like:
const showDataQuality = nothingIsNonZero || whitespaceIsNonZero
app/gui/src/project-view/components/visualizations/TableVisualization.vue
Outdated
Show resolved
Hide resolved
| <div style="visibility: ${getVisibility(params.numberOfNothing)};"> | ||
| Nulls/Nothing: ${getPercentage(params.numberOfNothing)}% ${createIndicator(+getPercentage(params.numberOfWhitespace))} | ||
| </div> |
There was a problem hiding this comment.
I think I'd hide this with display:none rather than visibility:hidden--if we leave a gap when the value is 0 or not applicable, I don't think it would be obvious why the gap is present.
| `<span style='display:flex; flex-direction:row; justify-content:space-between; width:inherit;'><span ref="eLabel" class="ag-header-cell-label" role="presentation" style='display:flex; flex-direction:row; justify-content:space-between; width:inherit;'> ${name} </span> ${menu} ${sort} ${getSvgTemplate(icon)} ${svgTemplateWarning}</span>` | ||
| : `<span ref="eLabel" style='display:flex; flex-direction:row; justify-content:space-between; width:inherit;'>${name} ${menu} ${sort} ${svgTemplateWarning}</span>` |
There was a problem hiding this comment.
These template lines are getting long. It would help to move some of the styling into classes.
jdunkerley
added
the
CI: Keep up to date
jdunkerley
added
CI: No changelog needed
jdunkerley
changed the title
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
or the Snowflake database integration, a run of the Extra Tests has been scheduled.