fix: show - for empty data in run table

[keita-determined]

Ticket

ET-80
Related PR: determined-ai/hew#135

Description

Empty entries in the run table should contain a dash

Test Plan

• If the cell value is empty (null value), show the dash - in the run table

Checklist

• Changes have been manually QA'd
• New features have been approved by the corresponding PM
• User-facing API changes have the "User-facing API Change" label
• Release notes have been added as a separate file under docs/release-notes/
  See Release Note for details.
• Licenses have been included for new code which was copied and/or modified from any external code

[codecov]

Codecov Report

Attention: Patch coverage is 7.86517% with 164 lines in your changes missing coverage. Please review.

Project coverage is 50.61%. Comparing base (ee269c8) to head (9866060).
Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
webui/react/src/pages/FlatRuns/columns.ts	7.34%	164 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9871      +/-   ##
==========================================
- Coverage   54.75%   50.61%   -4.14%     
==========================================
  Files        1261      938     -323     
  Lines      156333   126990   -29343     
  Branches     3600     3599       -1     
==========================================
- Hits        85604    64280   -21324     
+ Misses      70598    62579    -8019     
  Partials      131      131              

Flag	Coverage Δ
harness	?
web	54.52% <7.86%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
webui/react/src/components/RunActionDropdown.tsx	95.59% <100.00%> (ø)
webui/react/src/pages/FlatRuns/columns.ts	18.98% <7.34%> (+1.02%)	⬆️

... and 324 files with indirect coverage changes

[netlify]

✅ Deploy Preview for determined-ui ready!

Name	Link
🔨 Latest commit	9866060
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/66d1e6db2fa9aa00087ccb38
😎 Deploy Preview	https://deploy-preview-9871--determined-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[EmilyBonar]

• Forked column doesn't have dashes
• I think it would look better if the dashes were centered in the column, if possible
• It doesn't make much sense to allow users to right click and copy the "value" of - for an empty column. That's confusing behavior and it would be better if the dash was display only.

[keita-determined]

Forked column doesn't have dashes

fixed
Note: ideally, empty fork cell should be shown as text, but it's difficult to change the cell type when the data is empty. Even if there's no fork data, empty fork data, -, is shown as a link. The link doesn't go anywhere, and cursor icon is not a hand icon.

Screen.Recording.2024-08-30.at.11.01.23.PM.mov

I think it would look better if the dashes were centered in the column, if possible

it's difficult to make them centered.

It doesn't make much sense to allow users to right click and copy the "value" of - for an empty column. That's confusing behavior and it would be better if the dash was display only.

fixed. removed copy button if data is empty.

[EmilyBonar]

Forked column doesn't have dashes

fixed Note: ideally, empty fork cell should be shown as text, but it's difficult to change the cell type when the data is empty. Even if there's no fork data, empty fork data, -, is shown as a link. The link doesn't go anywhere, and cursor icon is not a hand icon.

I think it would look better if the dashes were centered in the column, if possible

it's difficult to make them centered.

Couldn't you create a EMPTY_CELL constant that looked something like:

{
      allowOverlay: false,
      data: '-',
      displayData: '-',
      kind: GridCellKind.Text,
      contentAlign: 'center'
}

And then you could just the following, which should fix both of the above problems:

renderer: (record: FlatRun) => (record.checkpointSize ? 
    {
      allowOverlay: false,
      copyData: humanReadableBytes(record.checkpointSize),
      data: { kind: TEXT_CELL },
      kind: GridCellKind.Custom,
    } : 
    EMPTY_CELL),

[keita-determined]

@EmilyBonar thats a good idea. made some changes.
not sure if we wanna do contentAlign: 'center' though. w&b and some others follows the column alignment. in our case, left aligned is the default. Also, centered dash might be harder to look when column width is long. I think we should ask product people first if we wanna make that move anyways.

[EmilyBonar]

I feel like an empty column marker being centered is fairly standard, but I suppose it doesn't matter. This looks good, but should it be applied to the Searches table as well?

[keita-determined]

This looks good, but should it be applied to the Searches table as well?

I think so. The ticket specifies run table, and i don't have enough capacities now. Made a ticket https://hpe-aiatscale.atlassian.net/browse/ET-749, so hope someone else can work on it.