Skip to content
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

Add user metadata #61

Merged
merged 6 commits into from
Aug 3, 2020
Merged

Add user metadata #61

merged 6 commits into from
Aug 3, 2020

Conversation

maeldebon
Copy link
Contributor

@maeldebon maeldebon commented Jul 23, 2020

This PR aims to add assets' metadata field to the frontend.

These metadata are currently displayed as follows.
When there are metadata:
Screenshot 2020-07-24 at 8 58 40 AM

When there are no metadata:
Screenshot 2020-07-24 at 8 59 31 AM

Copy link
Contributor

@jmorel jmorel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the code, it looks like you're missing the MetadataMetadata in testtupleSummary.js

const keys = Object.keys(metadata);

const li = css`
font-family: SFMono-Regular,Consolas,Liberation Mono,Menlo,Courier,monospace
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a monospaceFamily variable available that you should use instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, thanks!

@maeldebon maeldebon requested a review from jmorel July 23, 2020 10:00
@jmorel
Copy link
Contributor

jmorel commented Jul 23, 2020

@maeldebon could you include a screenshot of your recent changes?

@samlesu
Copy link
Contributor

samlesu commented Jul 24, 2020

@maeldebon could you include a screenshot of your recent changes?

That would be great!

@maeldebon
Copy link
Contributor Author

@maeldebon could you include a screenshot of your recent changes?

I updated the description

@@ -160,6 +169,27 @@ OwnerMetadata.defaultProps = {
owner: '',
};

export const MetadataMetadata = ({metadata}) => {
const keys = Object.keys(metadata);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're missing sorted() here as well :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes indeed!

@@ -35,10 +35,13 @@ const baseDd = css`

const metadataDt = css`
text-transform: initial;
font-family: ${monospaceFamily};
margin-left: ${spacingSmall};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use padding-left here, it would avoid having to specify a negative left margin the metadataDd class.

{keys.sort().map((key) => (
<SingleMetadata labelClassName={metadataDt} valueClassName={metadataDd} key={key} label={key} value={metadata[key]} />))}
{keys.map((key) => (
<SingleMetadata labelClassName={metadataDt} valueClassName={metadataDd} key={key} label={`- ${key}`} value={metadata[key]} />))}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Including the - like this works visually, but it introduces an unwanted behavior: When selecting the key, you'll also select the -. I guess this will also be picked up by screen readers.

One solution could be to have the metadataDt updated with:

 &:before {
  content: '- ';
}

Of course, the best solution would be to have an actual list, but in order to keep this short, the above solution should be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, it will be better this way

Copy link
Contributor

@jmorel jmorel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said in a previous review, it looks like you're missing the MetadataMetadata in testtupleSummary.js, but this is otherwise very nice!

@jmorel
Copy link
Contributor

jmorel commented Jul 27, 2020

Hello @maeldebon, since you're not available this week I took the liberty to add metadata to the TesttupleSummary and to fix a couple of bugs:

  • one related to this PR, for model, the metadata are stored in item.traintuple.metadata instead of item.metadata
  • one not related to this PR, the frontend exploded (nothing rendered at all) when accessing a model based on a composite traintuple that was not in status done.

Excellent work otherwise!

@maeldebon maeldebon merged commit 70bf27d into master Aug 3, 2020
@maeldebon maeldebon deleted the add_user_metadata branch August 3, 2020 08:09
acellard pushed a commit that referenced this pull request Sep 5, 2022
* Add helm chart linting

* Fix typo

* Add building & linting to PR approval process

* Reduce validate-pr length

* Auto-format

* fix build

* name steps

* Separate build job in validate-pr for speed

* Fix validate-pr workflow

Co-authored-by: Jérémy Morel <jeremy.morel@owkin.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants