[KED-2973] & [KED-2974] - fix metadata alignment and overlapping text #643
Conversation
| @@ -4,6 +4,10 @@ import { toHumanReadableTime } from '../../../utils/date-utils'; | |||
|
|
|||
| import './run-metadata.css'; | |||
|
|
|||
| // checks if value is an empty string, which is the default value returned | |||
| // by the graphql endpoint for empty values | |||
| const checkEmptyValue = (value) => (value !== '' ? value : '-'); | |||
There was a problem hiding this comment.
Nit: a check in my head returns a boolean. This has side effect, so I'd go for something like sanitiseEmptyValue. Feel free to ignore this comment though.
There was a problem hiding this comment.
Agree. I'd change the function name. I'd also remove the comment, as the name of the function itself is pretty self explanatory, IMO.
There was a problem hiding this comment.
Thanks both! I agree with the suggestion of the function naming.
I added the comment above particularly to provide clarity on the format that is returned from the graphql endpoint ( i.e I am checking for an empty string here given the current endpoint setup, and not checking for null or undefined values). I initially had the doubt to check for those other 2 situations but confirmed with the endpoint, and think this piece of information would be important in the assumption for the check executed in this function.
| @@ -4,6 +4,10 @@ import { toHumanReadableTime } from '../../../utils/date-utils'; | |||
|
|
|||
| import './run-metadata.css'; | |||
|
|
|||
| // checks if value is an empty string, which is the default value returned | |||
| // by the graphql endpoint for empty values | |||
| const checkEmptyValue = (value) => (value !== '' ? value : '-'); | |||
There was a problem hiding this comment.
Agree. I'd change the function name. I'd also remove the comment, as the name of the function itself is pretty self explanatory, IMO.
| @@ -31,53 +34,60 @@ const RunMetadata = ({ isSingleRun, runs = [] }) => { | |||
| className={classnames('details-metadata__run', { | |||
| 'details-metadata__run--single': isSingleRun, | |||
| })} | |||
| key={run.gitSha}> | |||
| key={run.title} | |||
There was a problem hiding this comment.
I'm not sure the run.title will always be unique, not like the gitSha was either. You may want to add the index to this for safety.
| key={run.title} | |
| key={run.title + index} |
There was a problem hiding this comment.
yea I had the exact same doubt as well, however I default this to the title given that the title now is the unique timestamp, moreover given that we have not implemented the mutation for the title which ensures this is unique.
The suggestion to add the index sounds good.
There was a problem hiding this comment.
update: I've added a comment that this value should be reverted to gitSha once the endpoint returns the actual value as it is still a better unique identifier.
| @@ -32,6 +32,7 @@ | |||
| font-size: 1.4em; | |||
| padding-bottom: 16px; | |||
| vertical-align: top; | |||
| word-wrap: break-word; // For breaking potential long text,such as as git branch, title, etc | |||
There was a problem hiding this comment.
There should be a space after "text,".
Description
resolves KED-2973 & KED-2974.
This PR is raised to fix the issues with the alignment of rows between runs, as well as the text formatting in the run details table.
Development notes
While looking into the issue of the row alignment of the table row for the metadata component, I realize that the empty fields within the metadata component are missing a '-' value as per the designs, hence I have added a small util function to check for the run metadata fields and apply the dash for empty values. This in turn will also fix the alignment issue.
I have modified the css to ensure that long text gets wrapped in the tracking dataset fields.
The comparison view now looks like this:
QA notes
This can be QAed by running this against your locally generated experiment data.
Checklist
RELEASE.mdfile