feat(ui): appset list page and filters #25837
Conversation
Signed-off-by: Peter Jiang <peterjiang823@gmail.com>
🔴 Preview Environment stopped on BunnyshellSee: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
Signed-off-by: Peter Jiang <peterjiang823@gmail.com>
Signed-off-by: Peter Jiang <peterjiang823@gmail.com>
Signed-off-by: Peter Jiang <peterjiang823@gmail.com>
| } | ||
|
|
||
| export function getFilterResults(applications: Application[], pref: AppsListPreferences): FilteredApp[] { | ||
| export function getFilterResults(applications: AbstractApplication[], pref: AbstractAppsListPreferences): AbstractFilteredApp[] { |
There was a problem hiding this comment.
Given that we are coercing pref to be AppsListPreferences in all its uses (pref as AppsListPreferences), would it be better to just keep its type as AppsListPreferences? Generally the as keyword is bad form because it breaks type safety.
| labels: pref.labelsFilter.length === 0 || pref.labelsFilter.every(selector => LabelSelector.match(selector, app.metadata.labels)), | ||
| operation: pref.operationFilter.length === 0 || pref.operationFilter.includes(getOperationStateTitle(app)) | ||
| } | ||
| filterResult: isApp(app) |
There was a problem hiding this comment.
same as with pref, since we validate that the object is an app, I don't think we should need to do app as Application through this map. Were there problems without those as statements?
| emptyState={() => ( | ||
| <EmptyState icon='fa fa-search'> | ||
| <h4>No matching applications found</h4> | ||
| <h4>No matching {isListOfApplications ? 'applications' : 'ApplicationSets'} found</h4> |
There was a problem hiding this comment.
nit: should this be "application sets" instead of "ApplicationSets"?
| const isOci = source?.repoURL?.startsWith('oci://'); | ||
| const targetRevision = source ? source.targetRevision || 'HEAD' : 'Unknown'; | ||
| const linkInfo = getApplicationLinkURL(app, ctx.baseHref); | ||
| const healthStatus = isApplication ? typedApp.status.health.status : getAppSetHealthStatus(typedAppSet); |
There was a problem hiding this comment.
I worry about the maintainability of some of these files. When we are filling a component with so many isApplication and !isApplication checks to make small modifications to the component, I wonder if it would be better to extract these into two separate components. i.e. an ApplicationTile and an AppSetTile.
We could leave these top-level declarations here, but instead of returning all these conditionally rendered sub-components, we can return either an ApplicationTile or an AppSetTile with self contained rendering logic that doesnt need to worry about what kind of tile it is.
| const typedApp = isApplication ? (app as models.Application) : null; | ||
| const typedAppSet = !isApplication ? (app as models.ApplicationSet) : null; | ||
| const healthStatus = isApplication ? typedApp.status.health.status : getAppSetHealthStatus(typedAppSet); | ||
| return ( |
There was a problem hiding this comment.
Same with the Application tiles, it could be good to extract all this conditional logic into self-contained components
2 participants
Checklist:
Currently it will be located at /applicationsets but we won't enable the route until the full feature is ready