Use two separate types for IChartCatalogItem and IOperatorCatalogItem. #1806
Conversation
| @@ -9,7 +9,7 @@ import { MessageAlert } from "../ErrorAlert"; | |||
| import LoadingWrapper from "../LoadingWrapper"; | |||
| import PageHeader from "../PageHeader"; | |||
| import SearchFilter from "../SearchFilter"; | |||
| import CatalogItem, { ICatalogItem } from "./CatalogItem"; | |||
| import CatalogItem, { ICatalogItemProps, IChartCatalogItem, IOperatorCatalogItem } from "./CatalogItem"; | |||
There was a problem hiding this comment.
The specific interfaces are only required for generating the segment of the key that differs depending on the type... not sure if that segment is required for uniqueness? (see below, line 106, where it includes either the csv or the repo name).
| const keyComponent = c.type === "operator" ? (c.item as IOperatorCatalogItem).csv : (c.item as IChartCatalogItem).repo.name; | ||
| return <CatalogItem type={c.type} key={`${c.type}/${keyComponent}/${c.item.name}`} item={c.item} /> |
There was a problem hiding this comment.
this seems unnecessary complex for just the key but I don't have a better proposal 🤔
There was a problem hiding this comment.
It's just for the required unique key, I think? so we could instead just use the index... I wasn't sure as I was only refactoring this code, so left it as it was - let me know if there's a reason for the more structured key?
| // Cosmetic change, remove the version from the csv name | ||
| const csvName = props.csv.split(".v")[0]; | ||
| const tag1 = <span>{csvName}</span>; | ||
| const link = `/ns/${namespace}/operators-instances/new/${csv}/${id}`; |
There was a problem hiding this comment.
this link should be obtained from a function :)
There was a problem hiding this comment.
Yeah - grepping for "/ns/.*operator" shows quite a few operator URLs in non-test code that aren't using the shared urls helper (or even have a url helper defined as in this case for the operator-instance new).
absoludity
force-pushed
the
1762-mc-ui-state
branch
2 times, most recently
from
1dce11e
to
1ab6f1a
Compare
It's not perfect - there's more that could be DRY'd up and encapsulated, but with this change you can actually use TypeScript's type system to ensure the correct fields are present. I've not done an IRL test, this is just to demo the thought. Fixes #1803 (maybe)
absoludity
force-pushed
the
1762-mc-ui-interlude
branch
from
a846f64
to
6e760a7
Compare
It's not perfect - there's more that could be DRY'd up and encapsulated,
but with this change you can actually use TypeScript's type system to
ensure the correct fields are present.
I've not done an IRL test, this is just to demo the thought.
Fixes #1803 (maybe). Ref #1762