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

fix(quay): Format date, size and Manifest information to be same as on the quay UI #101

Merged
merged 3 commits into from
Feb 9, 2023

Conversation

fmenesesg
Copy link
Collaborator

#100

Format date, size and Manifest information to be same as on the quay UI

Copy link
Member

@tumido tumido left a comment

Choose a reason for hiding this comment

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

There seems to be some formatting errors in here. Can you try running prettier please?

@fmenesesg
Copy link
Collaborator Author

I execute prettier, now should be ok, can you PTAL

@fmenesesg fmenesesg requested a review from tumido February 8, 2023 19:00
Copy link
Member

@tumido tumido left a comment

Choose a reason for hiding this comment

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

LGTM overall, I've left couple of suggestions. Feel free to dismiss them, this is mergeable as is.

}

const i = Math.floor(Math.log(sizeInBytes) / Math.log(1024));
return `${Number((sizeInBytes / Math.pow(1024, i)).toFixed(2)) * 1} ${
Copy link
Member

Choose a reason for hiding this comment

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

Hm, can you explain me what is the * 1 magic about? I'm unable to find a functional difference.

  return `${Number((sizeInBytes / Math.pow(1024, i)).toFixed(2)) * 1} ${
                                                                 ^^^^


const i = Math.floor(Math.log(sizeInBytes) / Math.log(1024));
return `${Number((sizeInBytes / Math.pow(1024, i)).toFixed(2)) * 1} ${
['B', 'kB', 'MB', 'GB', 'TB'][i]
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be better if this array was a global constant? I may be too picky here, just a nit.

Comment on lines +3 to +18
export function getSeverityColor(severity: VulnerabilitySeverity) {
switch (severity) {
case VulnerabilitySeverity.Critical:
return '#7D1007';
case VulnerabilitySeverity.High:
return '#C9190B';
case VulnerabilitySeverity.Medium:
return '#EC7A08';
case VulnerabilitySeverity.Low:
return '#F0AB00';
case VulnerabilitySeverity.None:
return '#3E8635';
default:
return '#8A8D90';
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I think a constant would be nice here, but I may be too picky:

Suggested change
export function getSeverityColor(severity: VulnerabilitySeverity) {
switch (severity) {
case VulnerabilitySeverity.Critical:
return '#7D1007';
case VulnerabilitySeverity.High:
return '#C9190B';
case VulnerabilitySeverity.Medium:
return '#EC7A08';
case VulnerabilitySeverity.Low:
return '#F0AB00';
case VulnerabilitySeverity.None:
return '#3E8635';
default:
return '#8A8D90';
}
}
export function getSeverityColor(severity: VulnerabilitySeverity) {
export const SEVERITY_COLORS = new Proxy(
{
VulnerabilitySeverity.Critical: '#7D1007',
VulnerabilitySeverity.High: '#C9190B',
VulnerabilitySeverity.Medium: '#EC7A08',
VulnerabilitySeverity.Low: '#F0AB00',
VulnerabilitySeverity.None: '#3E8635'
},
{
get: (o, k) => k in o ? o[k] : '#8A8D90'
}
)

This should be easily addressable via SEVERITY_COLORS[severity].

Again, just a nit, if you don't think agree, feel free to leave as it is, your implementation is completely reasonable and understandable.

Copy link
Member

@tumido tumido left a comment

Choose a reason for hiding this comment

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

After DM discussion the nits are filed as a separate refactor issue. It's minor changes anyways... We can merge this as is. LGTM 👍

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.

None yet

2 participants