-
Notifications
You must be signed in to change notification settings - Fork 198
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
Task 107 overview sources card #346
Task 107 overview sources card #346
Conversation
@@ -0,0 +1,5 @@ | |||
export const KIND_COLORS = { | |||
deployment: "#203548", | |||
daemonSet: "#033869", |
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.
daemonSet: "#033869", | |
daemonset: "#033869", |
@@ -0,0 +1,7 @@ | |||
export const SOURCES_LOGOS = { |
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 is the logo of programming languages, not sources
import SourceManagedCard from "./sources.manage.card"; | ||
|
||
export function SourcesManagedList({ data = [1, 1, 1, 1] }) { | ||
function renderDestinations() { |
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.
destinations -> sources
|
||
export function SourcesManagedList({ data = [1, 1, 1, 1] }) { | ||
function renderDestinations() { | ||
return data.map((source: any) => <SourceManagedCard />); |
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.
don't we need a react key prop for this list?
} | ||
|
||
return ( | ||
<> |
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.
redundant
return ( | ||
<> | ||
<ManagedListWrapper> | ||
{data?.length === 0 ? ( |
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.
id data
is undefined
this condition will be falsy and we will fallback to render the list which will throw
import Empty from "@/assets/images/empty-list.svg"; | ||
import SourceManagedCard from "./sources.manage.card"; | ||
|
||
export function SourcesManagedList({ data = [1, 1, 1, 1] }) { |
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.
What is [1, 1, 1, 1]
? Should we type it as ManagedSource[]
?
return ( | ||
<CardWrapper> | ||
<KeyvalImage | ||
src={SOURCES_LOGOS[item?.languages[0].language || ""]} |
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.
Do we want to always show the first one? what if there are many?
</KeyvalText> | ||
<KeyvalTag | ||
title={item?.kind || ""} | ||
color={KIND_COLORS[item?.kind.toLowerCase() || DEPLOYMENT]} |
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.
if kind
is undefined, this will throw.
You can also optional chining the kind:
item?.kind?.toLowerCase()
const TEXT_STYLE: React.CSSProperties = { | ||
textOverflow: "ellipsis", | ||
whiteSpace: "nowrap", | ||
overflow: "hidden", | ||
width: 224, | ||
textAlign: "center", | ||
}; | ||
const LOGO_STYLE: React.CSSProperties = { | ||
padding: 4, | ||
backgroundColor: theme.colors.white, | ||
}; |
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.
Can we use styled components instead?
<KeyvalTag title={item.kind} color={KIND_COLORS[item.kind]} /> | ||
<KeyvalTag | ||
title={item.kind} | ||
color={KIND_COLORS[item.kind].toLowerCase()} |
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.
Current implementation will lower case the color ("#203548".toLowerCase()
) and not the kind.
color={KIND_COLORS[item.kind].toLowerCase()} | |
color={KIND_COLORS[item.kind.toLowerCase()]} |
No description provided.