renames Chart list to Catalog list #788
Conversation
| "/charts": ChartListContainer, | ||
| "/charts/:repo": ChartListContainer, | ||
| "/catalog": CatalogListContainer, | ||
| "/catalog/:repo": CatalogListContainer, |
There was a problem hiding this comment.
For now, I'm not changing the ChartView routes, but this looks a little bit confusing. One option is to not change this to /catalog/:repo and keep it as /charts/:repo, but keep /catalog, but then the container is called CatalogListContainer. I'm not sure what the best option here is.
There was a problem hiding this comment.
I think that Catalog has the same meaning than ChartList so having CatalogList is a bit redundant (and maybe misleading since it can be understood as a list of catalogs). If we use that naming I think it has more sense to have:
"/catalog": CatalogContainer,
"/charts/:repo/:id": ChartViewContainer,
In any case, I wouldn't see weird having "/catalog/:repo/:id": ChartViewContainer either since it makes sense.
There was a problem hiding this comment.
Good point, will change this to CatalogContainer and the component to just Catalog
| @@ -32,3 +32,11 @@ | |||
| .ListItem__content__info_tag-2 { | |||
| background-color: #1598cb; | |||
| } | |||
|
|
|||
| .ListItem__content__info_tag.stable { | |||
There was a problem hiding this comment.
I think it's better to keep this tag in the specific CSS for the catalog. This card is meant to be generic so having here a class for "stable" and "incubator" is not the best. Mostly because there other components that uses the same card with other CSS classes like "provisioned" for the svc catalog cards or "failed". If we add them all here it would be difficult to maintain.
There was a problem hiding this comment.
The issue was that it's also being imported by the ServiceCatalogList components, but maybe since they will eventually be combined we can keep it as it was.
| "/charts": ChartListContainer, | ||
| "/charts/:repo": ChartListContainer, | ||
| "/catalog": CatalogListContainer, | ||
| "/catalog/:repo": CatalogListContainer, |
There was a problem hiding this comment.
I think that Catalog has the same meaning than ChartList so having CatalogList is a bit redundant (and maybe misleading since it can be understood as a list of catalogs). If we use that naming I think it has more sense to have:
"/catalog": CatalogContainer,
"/charts/:repo/:id": ChartViewContainer,
In any case, I wouldn't see weird having "/catalog/:repo/:id": ChartViewContainer either since it makes sense.
| @@ -8,7 +8,7 @@ import { CardGrid } from "../Card"; | |||
| import { MessageAlert } from "../ErrorAlert"; | |||
| import PageHeader from "../PageHeader"; | |||
| import SearchFilter from "../SearchFilter"; | |||
| import ChartList from "./ChartList"; | |||
| import CatalogList from "./CatalogList"; | |||
There was a problem hiding this comment.
I'd rename this to actually AppsCatalog so we do not overload Catalog and leave the door open for other kind of catalogs :)
There was a problem hiding this comment.
@migmartri the idea is that eventually we will include the ServiceCatalog classes in the Catalog view as well, right? In that case, it won't make sense for it to be called AppsCatalog, but I don't mind calling it that for now and then changing it when we get to that point.
There was a problem hiding this comment.
That's a good point. Catalog is fine then. Thanks!
- renames navigation link and page heading "Charts" -> "Catalog" - renames ChartsList Container and components to CatalogList - renames /charts and /charts/:repo routes to /catalog and /catalog/:repo - removes ChartsListItem.scss and moves the shared CSS rules to InfoCard
prydonius
force-pushed
the
761-charts-tog-catalog
branch
from
ddd44d7 to
ebac6da
Compare
fixes #761