Use plot errors to display correct messages for missing plots #3520
Conversation
![qlty-cloud-legacy[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
| <> | ||
| <ErrorTooltip error={error}> | ||
| <div> | ||
| <Error height={48} width={48} className={styles.errorIcon} /> |
There was a problem hiding this comment.
[F] Tried an $error-color ! as well, couldn't make it work.
There was a problem hiding this comment.
Did you set it like this:
.errorIcon {
color: $error-color;
margin: 6px;
svg {
fill: $error-color;
}
}
Else you can pass the color directly on the fill prop for the icon.
There was a problem hiding this comment.
sorry, there is no ! icon but we show ! for errors in the experiment table. What I meant was I had ! instead of the error icon for an error but it just looked weird (unfortunately).
| import { Error } from '../../../shared/components/icons' | ||
| import Tooltip from '../../../shared/components/tooltip/Tooltip' | ||
| import { Error } from '../icons' | ||
| import Tooltip from '../tooltip/Tooltip' |
There was a problem hiding this comment.
[Q] Should this component be grouped under tooltip?
There was a problem hiding this comment.
I think that would be a good idea. For discoverability mainly.
There was a problem hiding this comment.
I'll move in a follow up
| import React from 'react' | ||
| import { CellContents } from './CellContent' | ||
| import { ErrorTooltip } from '../Errors' | ||
| import { ErrorTooltip } from '../../../../shared/components/errorTooltip/ErrorTooltip' |
There was a problem hiding this comment.
[F] Moved this because we are using it in both the experiments table and plots now (also means it will be easier to make it look better everywhere).
| return undefined | ||
| } | ||
|
|
||
| return msgs.join('\n') |
There was a problem hiding this comment.
Should this be done when displaying instead? I feel like this could be more flexible if it were an array (displaying only the first error, wrap each error with something...). If it doesn't suit our use cases, it's totally fine to keep as is.
There was a problem hiding this comment.
Yes, you are right. I will move that logic out to the client.
There was a problem hiding this comment.
I will follow-up
| <> | ||
| <ErrorTooltip error={error}> | ||
| <div> | ||
| <Error height={48} width={48} className={styles.errorIcon} /> |
There was a problem hiding this comment.
Did you set it like this:
.errorIcon {
color: $error-color;
margin: 6px;
svg {
fill: $error-color;
}
}
Else you can pass the color directly on the fill prop for the icon.
| /> | ||
| </> | ||
| ) : ( | ||
| <p className={styles.emptyIcon}>-</p> |
There was a problem hiding this comment.
Why don't we keep <p>No Plot to Display.</p>?
There was a problem hiding this comment.
I stared at/reworded the statement for ~30 minutes then I decided that this is a table of data and that we could just display the same missing value that is displayed in the CLI experiments table. Could be the wrong decision but we can iterate with feedback 🙏🏻.
| import { Error } from '../../../shared/components/icons' | ||
| import Tooltip from '../../../shared/components/tooltip/Tooltip' | ||
| import { Error } from '../icons' | ||
| import Tooltip from '../tooltip/Tooltip' |
There was a problem hiding this comment.
I think that would be a good idea. For discoverability mainly.
|
I like the look and feel of this iteration way better, thank @mattseddon ! |
| /> | ||
| {error ? ( | ||
| <> | ||
| <ErrorTooltip error={error}> |
There was a problem hiding this comment.
Are users going to want to be able to copy the error? I know that when I run into an issue that I can't figure out I need to copy the error message so I can post it somewhere 🤔
There was a problem hiding this comment.
I will make this copyable in the follow-up where I move the component.
mattseddon
force-pushed
the
use-errors
branch
2 times, most recently
from
e20c9e1 to
869e560
Compare
|
Code Climate has analyzed commit 224cfbb and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 97.3% (85% is the threshold). This pull request will bring the total coverage in the repository to 95.1% (0.0% change). View more on Code Climate. |
4 participants
2/4
main<- #3477 <- this <- #3522 <- #3532Part of #2277
Screenshot
with tooltip shown
I tried to make a
!work to fit in with the errors shown in the experiments table but it did not look very good.Note: If an image is completely broken/missing for all revisions then we do not know that it is an image so we won't be able to display it like this. We will still be able to add an error indicator into the tree for this.
Previous idea: