pierrejeambrun commented on code in PR #45215:
URL: https://github.com/apache/airflow/pull/45215#discussion_r1905665151
##########
airflow/ui/src/pages/Run/TaskInstances.tsx:
##########
@@ -82,12 +85,71 @@ const columns: Array<ColumnDef<TaskInstanceResponse>> = [
},
];
+const stateOptions = createListCollection({
+ items: [
+ { label: "All States", value: "all" },
+ { label: "Scheduled", value: "scheduled" },
+ { label: "Queued", value: "queued" },
+ { label: "Running", value: "running" },
+ { label: "Success", value: "success" },
+ { label: "Restarting", value: "restarting" },
+ { label: "Failed", value: "failed" },
+ { label: "Up For Retry", value: "up_for_retry" },
+ { label: "Up For Reschedule", value: "up_for_reschedule" },
+ { label: "Upstream failed", value: "upstream_failed" },
+ { label: "Skipped", value: "skipped" },
+ { label: "Deferred", value: "deferred" },
+ { label: "Removed", value: "removed" },
Review Comment:
I think a task instance can also have `no status` => state is `null`, not
yet scheduled.
##########
airflow/ui/src/pages/Run/TaskInstances.tsx:
##########
@@ -82,12 +85,71 @@ const columns: Array<ColumnDef<TaskInstanceResponse>> = [
},
];
+const stateOptions = createListCollection({
Review Comment:
By adding typing here you will be able to remove manual casting `as
TaskInstanceState`. (And also by handling the None).
```
createListCollection<{ label: string; value: TaskInstanceState | "all" }
```
##########
airflow/ui/src/pages/Run/TaskInstances.tsx:
##########
@@ -82,12 +85,71 @@ const columns: Array<ColumnDef<TaskInstanceResponse>> = [
},
];
+const stateOptions = createListCollection({
+ items: [
+ { label: "All States", value: "all" },
Review Comment:
I believe this can be further simplified, by having nothing selected at all
'undefined' value, that is considered `all` implicitly so we do not have to
manually handle an "all". Because that also mess up the typing to have that
extra virtual 'state'.
##########
airflow/ui/src/components/SearchBar.tsx:
##########
@@ -61,9 +69,11 @@ export const SearchBar = ({ buttonProps, defaultValue,
groupProps, onChange, pla
size="xs"
/>
) : undefined}
- <Button fontWeight="normal" height="1.75rem" variant="ghost"
width={140} {...buttonProps}>
- Advanced Search
- </Button>
+ {Boolean(hideAdvanced) ? undefined : (
+ <Button fontWeight="normal" height="1.75rem" variant="ghost"
width={140} {...buttonProps}>
+ Advanced Search
+ </Button>
+ )}
Review Comment:
I am not aware of a SearchBar that actually leverage this feature. (with a
working button).
As you mentioned the description. This can be removed if unused.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
