-
Notifications
You must be signed in to change notification settings - Fork 359
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
fix: show -
for empty data in searches table [ET-749]
#9963
Conversation
✅ Deploy Preview for determined-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9963 +/- ##
==========================================
- Coverage 54.46% 50.46% -4.00%
==========================================
Files 1252 944 -308
Lines 156703 128071 -28632
Branches 3600 3602 +2
==========================================
- Hits 85341 64627 -20714
+ Misses 71229 63311 -7918
Partials 133 133
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added some comments, otherwise lgtm
can we update the test plan for the release party? and e2e ci failed |
Requesting re-review due to significant changes. Open for feedback on argument/variable names, I was having trouble thinking of ones that were descriptive. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added one comment, otherwise lgtm
webui/react/src/utils/table.ts
Outdated
cell: (datum: T) => GridCell, | ||
onlyUndefined = true, | ||
): GridCell => { | ||
if ((onlyUndefined && param === undefined) || (!onlyUndefined && !param)) return EMPTY_CELL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this works with the current columns, but if we wanna show EMPTY_CELL
for special cases, this wouldnt work. For example, show -
when data is EMPTY
or -1
etc. i feel like its safer not to use handleEmptyCell
. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that seems unlikely, but in that case we could either update handleEmptyCell
to deal with it or handle the special case when we call it, like handleEmptyCell(variable !== 'EMPTY' ? variable : undefined, ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it
Ticket
ET-749
Description
Test Plan
Make sure Flat Runs feature flag is on. Go into any project with a significant number of experiments (Uncategorized is a good default choice). Perform the following steps on both the Runs and Searches tabs.
-
presentChecklist
docs/release-notes/
See Release Note for details.