Re: [PR] Migrate trigger form params to new UI [airflow]
jscheffl merged PR #45270: URL: https://github.com/apache/airflow/pull/45270 -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Migrate trigger form params to new UI [airflow]
bbovenzi commented on code in PR #45270:
URL: https://github.com/apache/airflow/pull/45270#discussion_r1918775212
##
airflow/ui/src/components/FlexibleForm/NormalRow.tsx:
##
@@ -0,0 +1,49 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Field, Stack } from "@chakra-ui/react";
+import Markdown from "react-markdown";
+import remarkGfm from "remark-gfm";
+
+import type { ParamSpec } from "src/queries/useDagParams";
+
+import type { FlexibleFormElementProps } from ".";
+import { SelectElement } from "./SelectElement";
+
+const isRequired = (param: ParamSpec) =>
+ // The field is required if the schema type is defined.
+ // But if the type "null" is included, then the field is not required.
+ // We assume that "null" is only defined if the type is an array.
+ Boolean(param.schema.type) && (!Array.isArray(param.schema.type) ||
!param.schema.type.includes("null"));
+
+/** Render a normal form row with a field that is auto-selected */
+export const NormalRow = ({ key, name, param }: FlexibleFormElementProps) => (
Review Comment:
FormRow is a good descriptor!
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Migrate trigger form params to new UI [airflow]
bbovenzi commented on code in PR #45270:
URL: https://github.com/apache/airflow/pull/45270#discussion_r1918774423
##
airflow/ui/src/components/FlexibleForm/NormalRow.tsx:
##
@@ -0,0 +1,49 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Field, Stack } from "@chakra-ui/react";
+import Markdown from "react-markdown";
+import remarkGfm from "remark-gfm";
+
+import type { ParamSpec } from "src/queries/useDagParams";
+
+import type { FlexibleFormElementProps } from ".";
+import { SelectElement } from "./SelectElement";
+
+const isRequired = (param: ParamSpec) =>
+ // The field is required if the schema type is defined.
+ // But if the type "null" is included, then the field is not required.
+ // We assume that "null" is only defined if the type is an array.
+ Boolean(param.schema.type) && (!Array.isArray(param.schema.type) ||
!param.schema.type.includes("null"));
+
+/** Render a normal form row with a field that is auto-selected */
+export const NormalRow = ({ key, name, param }: FlexibleFormElementProps) => (
+
+
+
+{param.schema.title ?? name}
+
+
+
Review Comment:
Ok let's leave it as-sis then
##
airflow/ui/src/components/FlexibleForm/NormalRow.tsx:
##
@@ -0,0 +1,49 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Field, Stack } from "@chakra-ui/react";
+import Markdown from "react-markdown";
+import remarkGfm from "remark-gfm";
+
+import type { ParamSpec } from "src/queries/useDagParams";
+
+import type { FlexibleFormElementProps } from ".";
+import { SelectElement } from "./SelectElement";
+
+const isRequired = (param: ParamSpec) =>
+ // The field is required if the schema type is defined.
+ // But if the type "null" is included, then the field is not required.
+ // We assume that "null" is only defined if the type is an array.
+ Boolean(param.schema.type) && (!Array.isArray(param.schema.type) ||
!param.schema.type.includes("null"));
+
+/** Render a normal form row with a field that is auto-selected */
+export const NormalRow = ({ key, name, param }: FlexibleFormElementProps) => (
+
+
+
+{param.schema.title ?? name}
+
+
+
Review Comment:
Ok let's leave it as-is then
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Migrate trigger form params to new UI [airflow]
jscheffl commented on code in PR #45270:
URL: https://github.com/apache/airflow/pull/45270#discussion_r1917468814
##
airflow/ui/src/components/FlexibleForm/FieldDateTime.tsx:
##
@@ -0,0 +1,42 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Input } from "@chakra-ui/react";
+
+import type { FlexibleFormElementProps } from ".";
+
+export type DateTimeElementProps = {
+ readonly subType: string;
+} & FlexibleFormElementProps;
+
+const typeToPlaceholder: Record = {
+ date: "-mm-dd",
+ "datetime-local": "-mm-ddThh:mm",
+ time: "hh:mm",
+};
+
+export const FieldDateTime = ({ name, param, subType }: DateTimeElementProps)
=> (
Review Comment:
Just noticed that the placehodlers not needed at all for date+time pickers.
They are generated by the browser.
I removed the complexity.
And also realized that if I switch my system locale to German (I use EN-US
per default) then in Firefox the date and time placeholders are correct.
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Migrate trigger form params to new UI [airflow]
jscheffl commented on PR #45270: URL: https://github.com/apache/airflow/pull/45270#issuecomment-2594123650 > Found two things with the datetime field. > > Also, did you see my comment on renaming the Fields and NormalRow? Next round of reviews applied. Saw the request to rename "NormalRow" ... renamed it to proposed "FieldRow" - is this OK? The other comment about renaming the Fields... I did not understand. Can you help me what you wanted me to rename to? -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Migrate trigger form params to new UI [airflow]
jscheffl commented on code in PR #45270:
URL: https://github.com/apache/airflow/pull/45270#discussion_r1917452245
##
airflow/ui/src/components/FlexibleForm/FieldDateTime.tsx:
##
@@ -0,0 +1,42 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Input } from "@chakra-ui/react";
+
+import type { FlexibleFormElementProps } from ".";
+
+export type DateTimeElementProps = {
+ readonly subType: string;
+} & FlexibleFormElementProps;
+
+const typeToPlaceholder: Record = {
+ date: "-mm-dd",
+ "datetime-local": "-mm-ddThh:mm",
+ time: "hh:mm",
+};
+
+export const FieldDateTime = ({ name, param, subType }: DateTimeElementProps)
=> (
Review Comment:
Nope, the placeholders are hard-coded. Do you know any chakra localizable
option?
I just checked but also using @orange_digital/chakra-datepicker is not a
better option in my view... short research tells me... or I am blind - don't
see better components available?
Maybe we start the the US placeholders and rework later? Otherwise ideas
welcome. (not an expert in this :-( are there localization libs?)
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Migrate trigger form params to new UI [airflow]
jscheffl commented on code in PR #45270:
URL: https://github.com/apache/airflow/pull/45270#discussion_r1917445855
##
airflow/ui/src/components/FlexibleForm/NormalRow.tsx:
##
@@ -0,0 +1,49 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Field, Stack } from "@chakra-ui/react";
+import Markdown from "react-markdown";
+import remarkGfm from "remark-gfm";
+
+import type { ParamSpec } from "src/queries/useDagParams";
+
+import type { FlexibleFormElementProps } from ".";
+import { SelectElement } from "./SelectElement";
+
+const isRequired = (param: ParamSpec) =>
+ // The field is required if the schema type is defined.
+ // But if the type "null" is included, then the field is not required.
+ // We assume that "null" is only defined if the type is an array.
+ Boolean(param.schema.type) && (!Array.isArray(param.schema.type) ||
!param.schema.type.includes("null"));
+
+/** Render a normal form row with a field that is auto-selected */
+export const NormalRow = ({ key, name, param }: FlexibleFormElementProps) => (
+
+
+
+{param.schema.title ?? name}
+
+
+
Review Comment:
Yeah, one point is right - the definition with 70% is sufficient!
But if I change from `` to ``
The form switches to
Reverting back to the ugly/complex/indirect CSS statement it makes it
clean/pretty. Maybe I am just too stupid but with your proposed approach the
inspector does not show the `flex-basis` at the generated `` at all :-O
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Migrate trigger form params to new UI [airflow]
jscheffl commented on code in PR #45270:
URL: https://github.com/apache/airflow/pull/45270#discussion_r1917434041
##
airflow/ui/src/components/FlexibleForm/FieldSelector.tsx:
##
@@ -0,0 +1,118 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import type { ParamSchema, ParamSpec } from "src/queries/useDagParams";
+
+import type { FlexibleFormElementProps } from ".";
+import { FieldAdvancedArray } from "./FieldAdvancedArray";
+import { FieldBool } from "./FieldBool";
+import { FieldDateTime } from "./FieldDateTime";
+import { FieldDropdown } from "./FieldDropdown";
+import { FieldMultiSelect } from "./FieldMultiSelect";
+import { FieldMultilineText } from "./FieldMultilineText";
+import { FieldNumber } from "./FieldNumber";
+import { FieldObject } from "./FieldObject";
+import { FieldString } from "./FieldString";
+import { FieldStringArray } from "./FieldStringArray";
+
+const inferType = (param: ParamSpec) => {
+ if (Boolean(param.schema.type)) {
+// If there are multiple types, we assume that the first one is the
correct one that is not "null".
+// "null" is only used to signal the value is optional.
+if (Array.isArray(param.schema.type)) {
+ return param.schema.type.find((type) => type !== "null") ?? "string";
+}
+
+return param.schema.type ?? "string";
+ }
+
+ // If the type is not defined, we infer it from the value.
+ if (Array.isArray(param.value)) {
+return "array";
+ }
+
+ return typeof param.value;
+};
+
+const isFieldAdvancedArray = (fieldType: string, fieldSchema: ParamSchema) =>
+ fieldType === "array" && fieldSchema.items?.type !== "string";
+
+const isFieldBool = (fieldType: string) => fieldType === "boolean";
+
+const isFieldDate = (fieldType: string, fieldSchema: ParamSchema) =>
+ fieldType === "string" && fieldSchema.format === "date";
+
+const isFieldDateTime = (fieldType: string, fieldSchema: ParamSchema) =>
+ fieldType === "string" && fieldSchema.format === "date-time";
+
+const enumTypes = ["string", "number", "integer"];
+
+const isFieldDropdown = (fieldType: string, fieldSchema: ParamSchema) =>
+ enumTypes.includes(fieldType) && Array.isArray(fieldSchema.enum);
+
+const isFieldMultilineText = (fieldType: string, fieldSchema: ParamSchema) =>
+ fieldType === "string" && fieldSchema.format === "multiline";
+
+const isFieldMultiSelect = (fieldType: string, fieldSchema: ParamSchema) =>
+ fieldType === "array" && Array.isArray(fieldSchema.examples);
+
+const isFieldNumber = (fieldType: string) => {
+ const numberTypes = ["integer", "number"];
+
+ return numberTypes.includes(fieldType);
+};
+
+const isFieldObject = (fieldType: string) => fieldType === "object";
+
+const isFieldStringArray = (fieldType: string, fieldSchema: ParamSchema) =>
+ fieldType === "array" &&
+ (!fieldSchema.items || fieldSchema.items.type === undefined ||
fieldSchema.items.type === "string");
+
+const isFieldTime = (fieldType: string, fieldSchema: ParamSchema) =>
+ fieldType === "string" && fieldSchema.format === "date";
Review Comment:
Great catch!
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Migrate trigger form params to new UI [airflow]
bbovenzi commented on code in PR #45270:
URL: https://github.com/apache/airflow/pull/45270#discussion_r1915883103
##
airflow/ui/src/components/FlexibleForm/FieldDateTime.tsx:
##
@@ -0,0 +1,42 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Input } from "@chakra-ui/react";
+
+import type { FlexibleFormElementProps } from ".";
+
+export type DateTimeElementProps = {
+ readonly subType: string;
+} & FlexibleFormElementProps;
+
+const typeToPlaceholder: Record = {
+ date: "-mm-dd",
+ "datetime-local": "-mm-ddThh:mm",
+ time: "hh:mm",
+};
+
+export const FieldDateTime = ({ name, param, subType }: DateTimeElementProps)
=> (
Review Comment:
```suggestion
export const FieldDateTime = ({ name, param, ...rest }:
FlexibleFormElementProps & InputProps) => (
```
we can just use the existing InputProps. And chraka's placeholders are
actually fine:
https://github.com/user-attachments/assets/946f46df-a415-4496-95d7-5cab138705c7";
/>
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Migrate trigger form params to new UI [airflow]
bbovenzi commented on code in PR #45270:
URL: https://github.com/apache/airflow/pull/45270#discussion_r1915886203
##
airflow/ui/src/components/FlexibleForm/FieldDateTime.tsx:
##
@@ -0,0 +1,42 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Input } from "@chakra-ui/react";
+
+import type { FlexibleFormElementProps } from ".";
+
+export type DateTimeElementProps = {
+ readonly subType: string;
+} & FlexibleFormElementProps;
+
+const typeToPlaceholder: Record = {
+ date: "-mm-dd",
+ "datetime-local": "-mm-ddThh:mm",
+ time: "hh:mm",
+};
+
+export const FieldDateTime = ({ name, param, subType }: DateTimeElementProps)
=> (
Review Comment:
These are US defaults for me, are the placeholders different for you based
on your locale?
##
airflow/ui/src/components/FlexibleForm/FieldDateTime.tsx:
##
@@ -0,0 +1,42 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Input } from "@chakra-ui/react";
+
+import type { FlexibleFormElementProps } from ".";
+
+export type DateTimeElementProps = {
+ readonly subType: string;
+} & FlexibleFormElementProps;
+
+const typeToPlaceholder: Record = {
+ date: "-mm-dd",
+ "datetime-local": "-mm-ddThh:mm",
+ time: "hh:mm",
+};
+
+export const FieldDateTime = ({ name, param, subType }: DateTimeElementProps)
=> (
Review Comment:
These are US defaults for me, are the chakra placeholders different for you
based on your locale?
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Migrate trigger form params to new UI [airflow]
bbovenzi commented on code in PR #45270:
URL: https://github.com/apache/airflow/pull/45270#discussion_r1915883268
##
airflow/ui/src/components/FlexibleForm/FieldSelector.tsx:
##
@@ -0,0 +1,118 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import type { ParamSchema, ParamSpec } from "src/queries/useDagParams";
+
+import type { FlexibleFormElementProps } from ".";
+import { FieldAdvancedArray } from "./FieldAdvancedArray";
+import { FieldBool } from "./FieldBool";
+import { FieldDateTime } from "./FieldDateTime";
+import { FieldDropdown } from "./FieldDropdown";
+import { FieldMultiSelect } from "./FieldMultiSelect";
+import { FieldMultilineText } from "./FieldMultilineText";
+import { FieldNumber } from "./FieldNumber";
+import { FieldObject } from "./FieldObject";
+import { FieldString } from "./FieldString";
+import { FieldStringArray } from "./FieldStringArray";
+
+const inferType = (param: ParamSpec) => {
+ if (Boolean(param.schema.type)) {
+// If there are multiple types, we assume that the first one is the
correct one that is not "null".
+// "null" is only used to signal the value is optional.
+if (Array.isArray(param.schema.type)) {
+ return param.schema.type.find((type) => type !== "null") ?? "string";
+}
+
+return param.schema.type ?? "string";
+ }
+
+ // If the type is not defined, we infer it from the value.
+ if (Array.isArray(param.value)) {
+return "array";
+ }
+
+ return typeof param.value;
+};
+
+const isFieldAdvancedArray = (fieldType: string, fieldSchema: ParamSchema) =>
+ fieldType === "array" && fieldSchema.items?.type !== "string";
+
+const isFieldBool = (fieldType: string) => fieldType === "boolean";
+
+const isFieldDate = (fieldType: string, fieldSchema: ParamSchema) =>
+ fieldType === "string" && fieldSchema.format === "date";
+
+const isFieldDateTime = (fieldType: string, fieldSchema: ParamSchema) =>
+ fieldType === "string" && fieldSchema.format === "date-time";
+
+const enumTypes = ["string", "number", "integer"];
+
+const isFieldDropdown = (fieldType: string, fieldSchema: ParamSchema) =>
+ enumTypes.includes(fieldType) && Array.isArray(fieldSchema.enum);
+
+const isFieldMultilineText = (fieldType: string, fieldSchema: ParamSchema) =>
+ fieldType === "string" && fieldSchema.format === "multiline";
+
+const isFieldMultiSelect = (fieldType: string, fieldSchema: ParamSchema) =>
+ fieldType === "array" && Array.isArray(fieldSchema.examples);
+
+const isFieldNumber = (fieldType: string) => {
+ const numberTypes = ["integer", "number"];
+
+ return numberTypes.includes(fieldType);
+};
+
+const isFieldObject = (fieldType: string) => fieldType === "object";
+
+const isFieldStringArray = (fieldType: string, fieldSchema: ParamSchema) =>
+ fieldType === "array" &&
+ (!fieldSchema.items || fieldSchema.items.type === undefined ||
fieldSchema.items.type === "string");
+
+const isFieldTime = (fieldType: string, fieldSchema: ParamSchema) =>
+ fieldType === "string" && fieldSchema.format === "date";
Review Comment:
```suggestion
fieldType === "string" && fieldSchema.format === "time";
```
##
airflow/ui/src/components/FlexibleForm/FieldDateTime.tsx:
##
@@ -0,0 +1,42 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Input } from "@chakra-ui/react";
+
+import type { FlexibleFormElementProps } from ".";
+
+export type DateTimeElementProps = {
+ readonly subTyp
Re: [PR] Migrate trigger form params to new UI [airflow]
bbovenzi commented on code in PR #45270:
URL: https://github.com/apache/airflow/pull/45270#discussion_r1915877402
##
airflow/ui/src/components/FlexibleForm/NormalRow.tsx:
##
@@ -0,0 +1,49 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Field, Stack } from "@chakra-ui/react";
+import Markdown from "react-markdown";
+import remarkGfm from "remark-gfm";
+
+import type { ParamSpec } from "src/queries/useDagParams";
+
+import type { FlexibleFormElementProps } from ".";
+import { SelectElement } from "./SelectElement";
+
+const isRequired = (param: ParamSpec) =>
+ // The field is required if the schema type is defined.
+ // But if the type "null" is included, then the field is not required.
+ // We assume that "null" is only defined if the type is an array.
+ Boolean(param.schema.type) && (!Array.isArray(param.schema.type) ||
!param.schema.type.includes("null"));
+
+/** Render a normal form row with a field that is auto-selected */
+export const NormalRow = ({ key, name, param }: FlexibleFormElementProps) => (
+
+
+
+{param.schema.title ?? name}
+
+
+
Review Comment:
Actually we only need the `flexBasis="70%"` all the others can be removed.
but what are you seeing when you do that?
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Migrate trigger form params to new UI [airflow]
jscheffl commented on PR #45270: URL: https://github.com/apache/airflow/pull/45270#issuecomment-2591265505 > Sorry this took me a bit to review. > > Now I see how sections work with the accordions so ignore my previous comment. > > It is a bit of a nitpick, but coudl we rename the field components? Along the lines of `NumberInput` instead of `FieldNumber`? > > I'm excited we're finally able to migrate all of this to react! Great work! Uff, this was a very good review! Learned something new. I assume now it needs a second iteration review from the rework... Had some challenges with the `flex-basis` - which is much cleaner in your proposal... but somehow not working. Any idea why? -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Migrate trigger form params to new UI [airflow]
jscheffl commented on code in PR #45270:
URL: https://github.com/apache/airflow/pull/45270#discussion_r1915713736
##
airflow/ui/src/components/FlexibleForm/NormalRow.tsx:
##
@@ -0,0 +1,49 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Field, Stack } from "@chakra-ui/react";
+import Markdown from "react-markdown";
+import remarkGfm from "remark-gfm";
+
+import type { ParamSpec } from "src/queries/useDagParams";
+
+import type { FlexibleFormElementProps } from ".";
+import { SelectElement } from "./SelectElement";
+
+const isRequired = (param: ParamSpec) =>
+ // The field is required if the schema type is defined.
+ // But if the type "null" is included, then the field is not required.
+ // We assume that "null" is only defined if the type is an array.
+ Boolean(param.schema.type) && (!Array.isArray(param.schema.type) ||
!param.schema.type.includes("null"));
+
+/** Render a normal form row with a field that is auto-selected */
+export const NormalRow = ({ key, name, param }: FlexibleFormElementProps) => (
Review Comment:
With "Normal Row" I wanted to say that this is "not hidden" - maybe there
will be more complex field types int he future... "Normal" for me is a label
plus some entry.
We have too many components with "Fields" already... this here renders a
row. Would `FormRow` be also OK?
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Migrate trigger form params to new UI [airflow]
jscheffl commented on code in PR #45270:
URL: https://github.com/apache/airflow/pull/45270#discussion_r1915712961
##
airflow/ui/src/components/FlexibleForm/SelectElement.tsx:
##
@@ -0,0 +1,120 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import type { ParamSchema, ParamSpec } from "src/queries/useDagParams";
+
+import type { FlexibleFormElementProps } from ".";
+import { FieldAdvancedArray } from "./FieldAdvancedArray";
+import { FieldBool } from "./FieldBool";
+import { FieldDate } from "./FieldDate";
+import { FieldDateTime } from "./FieldDateTime";
+import { FieldDropdown } from "./FieldDropdown";
+import { FieldMultiSelect } from "./FieldMultiSelect";
+import { FieldMultilineText } from "./FieldMultilineText";
+import { FieldNumber } from "./FieldNumber";
+import { FieldObject } from "./FieldObject";
+import { FieldString } from "./FieldString";
+import { FieldStringArray } from "./FieldStringArray";
+import { FieldTime } from "./FieldTime";
+
+const inferType = (param: ParamSpec) => {
+ if (Boolean(param.schema.type)) {
+// If there are multiple types, we assume that the first one is the
correct one that is not "null".
+// "null" is only used to signal the value is optional.
+if (Array.isArray(param.schema.type)) {
+ return param.schema.type.find((type) => type !== "null") ?? "string";
+}
+
+return param.schema.type ?? "string";
+ }
+
+ // If the type is not defined, we infer it from the value.
+ if (Array.isArray(param.value)) {
+return "array";
+ }
+
+ return typeof param.value;
+};
+
+const isFieldAdvancedArray = (fieldType: string, fieldSchema: ParamSchema) =>
+ fieldType === "array" && fieldSchema.items?.type !== "string";
+
+const isFieldBool = (fieldType: string) => fieldType === "boolean";
+
+const isFieldDate = (fieldType: string, fieldSchema: ParamSchema) =>
+ fieldType === "string" && fieldSchema.format === "date";
+
+const isFieldDateTime = (fieldType: string, fieldSchema: ParamSchema) =>
+ fieldType === "string" && fieldSchema.format === "date-time";
+
+const enumTypes = ["string", "number", "integer"];
+
+const isFieldDropdown = (fieldType: string, fieldSchema: ParamSchema) =>
+ enumTypes.includes(fieldType) && Array.isArray(fieldSchema.enum);
+
+const isFieldMultilineText = (fieldType: string, fieldSchema: ParamSchema) =>
+ fieldType === "string" && fieldSchema.format === "multiline";
+
+const isFieldMultiSelect = (fieldType: string, fieldSchema: ParamSchema) =>
+ fieldType === "array" && Array.isArray(fieldSchema.examples);
+
+const isFieldNumber = (fieldType: string) => {
+ const numberTypes = ["integer", "number"];
+
+ return numberTypes.includes(fieldType);
+};
+
+const isFieldObject = (fieldType: string) => fieldType === "object";
+
+const isFieldStringArray = (fieldType: string, fieldSchema: ParamSchema) =>
+ fieldType === "array" &&
+ (!fieldSchema.items || fieldSchema.items.type === undefined ||
fieldSchema.items.type === "string");
+
+const isFieldTime = (fieldType: string, fieldSchema: ParamSchema) =>
+ fieldType === "string" && fieldSchema.format === "date";
+
+export const SelectElement = ({ key, name, param }: FlexibleFormElementProps)
=> {
Review Comment:
Named it ` FieldSelector` - is that OK and describes what it does?
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Migrate trigger form params to new UI [airflow]
jscheffl commented on code in PR #45270:
URL: https://github.com/apache/airflow/pull/45270#discussion_r1915710160
##
airflow/ui/src/components/FlexibleForm/index.tsx:
##
@@ -0,0 +1,91 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Stack, StackSeparator } from "@chakra-ui/react";
+
+import type { DagParamsSpec, ParamSpec } from "src/queries/useDagParams";
+
+import { Accordion, Alert } from "../ui";
+import { Row } from "./Row";
+
+type FlexibleFormProps = {
+ readonly params: DagParamsSpec;
+};
+
+export type FlexibleFormElementProps = {
+ readonly key: string;
+ readonly name: string;
+ readonly param: ParamSpec;
+};
+
+const FlexibleForm = ({ params }: FlexibleFormProps) => {
+ const processedSections = new Map();
+
+ return (
+<>
+ {Object.entries(params).some(([, param]) => typeof param.schema.section
!== "string") ? (
+
+ Run
Parameters
+
+}>
+ {Object.keys(params).length > 0 && (
+
+ )}
+ {Object.entries(params)
+.filter(([, param]) => typeof param.schema.section !==
"string")
+.map(([name, param]) => (
+
+))}
+
+
+
+ ) : undefined}
+ {Object.entries(params)
+.filter(([, secParam]) => secParam.schema.section)
+.map(([, secParam]) => {
+ const currentSection = secParam.schema.section;
+
+ if (processedSections.has(currentSection)) {
+return undefined;
+ } else {
+processedSections.set(currentSection, true);
+
+return (
+
+{secParam.schema.section}
+
+ }>
+{Object.entries(params)
+ .filter(([, param]) => param.schema.section ===
currentSection)
Review Comment:
Cool optimization! Did not get to this point. Was a hard point getting this
"compiled" but much better now! Thanks!
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Migrate trigger form params to new UI [airflow]
jscheffl commented on code in PR #45270:
URL: https://github.com/apache/airflow/pull/45270#discussion_r1915708116
##
airflow/ui/src/components/FlexibleForm/NormalRow.tsx:
##
@@ -0,0 +1,49 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Field, Stack } from "@chakra-ui/react";
+import Markdown from "react-markdown";
+import remarkGfm from "remark-gfm";
+
+import type { ParamSpec } from "src/queries/useDagParams";
+
+import type { FlexibleFormElementProps } from ".";
+import { SelectElement } from "./SelectElement";
+
+const isRequired = (param: ParamSpec) =>
+ // The field is required if the schema type is defined.
+ // But if the type "null" is included, then the field is not required.
+ // We assume that "null" is only defined if the type is an array.
+ Boolean(param.schema.type) && (!Array.isArray(param.schema.type) ||
!param.schema.type.includes("null"));
+
+/** Render a normal form row with a field that is auto-selected */
+export const NormalRow = ({ key, name, param }: FlexibleFormElementProps) => (
+
+
+
+{param.schema.title ?? name}
+
+
+
Review Comment:
I attempted the adjust the `flex-basis`as proposed but this destroyed the
layout... do you have an idea why? Just does NOT parse the flex setting to the
element when I inspect the outcome rendered :-(
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Migrate trigger form params to new UI [airflow]
jscheffl commented on code in PR #45270:
URL: https://github.com/apache/airflow/pull/45270#discussion_r1915707327
##
airflow/ui/src/components/FlexibleForm/index.tsx:
##
@@ -0,0 +1,91 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Stack, StackSeparator } from "@chakra-ui/react";
+
+import type { DagParamsSpec, ParamSpec } from "src/queries/useDagParams";
+
+import { Accordion, Alert } from "../ui";
+import { Row } from "./Row";
+
+type FlexibleFormProps = {
+ readonly params: DagParamsSpec;
+};
+
+export type FlexibleFormElementProps = {
+ readonly key: string;
Review Comment:
Learning something new... every day :-D
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Migrate trigger form params to new UI [airflow]
jscheffl commented on code in PR #45270:
URL: https://github.com/apache/airflow/pull/45270#discussion_r1915707085
##
airflow/ui/src/queries/useDagParams.ts:
##
@@ -19,10 +19,36 @@
import { useDagServiceGetDagDetails } from "openapi/queries";
import { toaster } from "src/components/ui";
+export type DagParamsSpec = Record;
+
+export type ParamSpec = {
+ description: string | null;
+ schema: ParamSchema;
+ value: unknown;
+};
+
+export type ParamSchema = {
+ const: string | null;
+ description_md: string | null;
+ enum: Array | null;
+ examples: Array | null;
+ format: string | null;
+ items: Record | null;
+ maximum: number | null;
+ maxLength: number | null;
+ minimum: number | null;
+ minLength: number | null;
+ section: string | null;
+ title: string | null;
+ type: Array | string | null;
+ values_display: Record | null;
+};
Review Comment:
Added a TODO. I am okay to check in a sepearate PR
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Migrate trigger form params to new UI [airflow]
jscheffl commented on code in PR #45270:
URL: https://github.com/apache/airflow/pull/45270#discussion_r1915706665
##
airflow/ui/src/components/FlexibleForm/SelectElement.tsx:
##
@@ -0,0 +1,120 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import type { ParamSchema, ParamSpec } from "src/queries/useDagParams";
+
+import type { FlexibleFormElementProps } from ".";
+import { FieldAdvancedArray } from "./FieldAdvancedArray";
+import { FieldBool } from "./FieldBool";
+import { FieldDate } from "./FieldDate";
+import { FieldDateTime } from "./FieldDateTime";
+import { FieldDropdown } from "./FieldDropdown";
+import { FieldMultiSelect } from "./FieldMultiSelect";
+import { FieldMultilineText } from "./FieldMultilineText";
+import { FieldNumber } from "./FieldNumber";
+import { FieldObject } from "./FieldObject";
+import { FieldString } from "./FieldString";
+import { FieldStringArray } from "./FieldStringArray";
+import { FieldTime } from "./FieldTime";
+
+const inferType = (param: ParamSpec) => {
+ if (Boolean(param.schema.type)) {
+// If there are multiple types, we assume that the first one is the
correct one that is not "null".
+// "null" is only used to signal the value is optional.
+if (Array.isArray(param.schema.type)) {
+ return param.schema.type.find((type) => type !== "null") ?? "string";
+}
+
+return param.schema.type ?? "string";
+ }
+
+ // If the type is not defined, we infer it from the value.
+ if (Array.isArray(param.value)) {
+return "array";
+ }
+
+ return typeof param.value;
+};
+
+const isFieldAdvancedArray = (fieldType: string, fieldSchema: ParamSchema) =>
+ fieldType === "array" && fieldSchema.items?.type !== "string";
+
+const isFieldBool = (fieldType: string) => fieldType === "boolean";
+
+const isFieldDate = (fieldType: string, fieldSchema: ParamSchema) =>
+ fieldType === "string" && fieldSchema.format === "date";
+
+const isFieldDateTime = (fieldType: string, fieldSchema: ParamSchema) =>
+ fieldType === "string" && fieldSchema.format === "date-time";
+
+const enumTypes = ["string", "number", "integer"];
+
+const isFieldDropdown = (fieldType: string, fieldSchema: ParamSchema) =>
+ enumTypes.includes(fieldType) && Array.isArray(fieldSchema.enum);
+
+const isFieldMultilineText = (fieldType: string, fieldSchema: ParamSchema) =>
+ fieldType === "string" && fieldSchema.format === "multiline";
+
+const isFieldMultiSelect = (fieldType: string, fieldSchema: ParamSchema) =>
+ fieldType === "array" && Array.isArray(fieldSchema.examples);
+
+const isFieldNumber = (fieldType: string) => {
+ const numberTypes = ["integer", "number"];
+
+ return numberTypes.includes(fieldType);
+};
+
+const isFieldObject = (fieldType: string) => fieldType === "object";
+
+const isFieldStringArray = (fieldType: string, fieldSchema: ParamSchema) =>
+ fieldType === "array" &&
+ (!fieldSchema.items || fieldSchema.items.type === undefined ||
fieldSchema.items.type === "string");
+
+const isFieldTime = (fieldType: string, fieldSchema: ParamSchema) =>
+ fieldType === "string" && fieldSchema.format === "date";
+
+export const SelectElement = ({ key, name, param }: FlexibleFormElementProps)
=> {
+ // FUTURE: Add support for other types as described in AIP-68 via Plugins
+ const fieldType = inferType(param);
+
+ if (isFieldBool(fieldType)) {
+return ;
+ } else if (isFieldDateTime(fieldType, param.schema)) {
+return ;
+ } else if (isFieldDate(fieldType, param.schema)) {
+return ;
+ } else if (isFieldTime(fieldType, param.schema)) {
+return ;
+ } else if (isFieldDropdown(fieldType, param.schema)) {
Review Comment:
Adjusted for Date/Time/DateTime.
But explicitly _not_ consolidated for Multiline text and StringArray... I
assume different logic is needed later for parsing. Let's see how this will go,
maybe later we can re-visit.
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Migrate trigger form params to new UI [airflow]
jscheffl commented on code in PR #45270: URL: https://github.com/apache/airflow/pull/45270#discussion_r1915705712 ## airflow/ui/src/components/FlexibleForm/index.tsx: ## Review Comment: Good point, adjusted -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Migrate trigger form params to new UI [airflow]
bbovenzi commented on code in PR #45270:
URL: https://github.com/apache/airflow/pull/45270#discussion_r1915513872
##
airflow/ui/src/components/FlexibleForm/FlexibleFormFieldObject.tsx:
##
@@ -0,0 +1,52 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { json } from "@codemirror/lang-json";
+import { githubLight, githubDark } from "@uiw/codemirror-themes-all";
+import CodeMirror from "@uiw/react-codemirror";
+
+import { useColorMode } from "src/context/colorMode";
+
+import type { FlexibleFormElementProps } from ".";
+
+export const FlexibleFormFieldObject = ({ name, param }:
FlexibleFormElementProps) => {
+ const { colorMode } = useColorMode();
+
+ return (
+
Re: [PR] Migrate trigger form params to new UI [airflow]
bbovenzi commented on code in PR #45270:
URL: https://github.com/apache/airflow/pull/45270#discussion_r1915097094
##
airflow/ui/src/components/TriggerDag/TriggerDAGForm.tsx:
##
@@ -205,7 +208,7 @@ const TriggerDAGForm = ({ dagId, onClose, open }:
TriggerDAGFormProps) => {
field.onChange(validateAndPrettifyJson(field.value));
}}
style={{
-border: "1px solid #CBD5E0",
+border: "1px solid var(--chakra-colors-border)",
Review Comment:
Oh, this is much better than what I suggested.
##
airflow/ui/src/components/FlexibleForm/FieldMultiSelect.tsx:
##
@@ -0,0 +1,60 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Select as ReactSelect } from "chakra-react-select";
+import { useState } from "react";
+
+import type { FlexibleFormElementProps } from ".";
+
+const labelLookup = (key: string, valuesDisplay: Record |
null): string => {
+ if (valuesDisplay && typeof valuesDisplay === "object") {
+return valuesDisplay[key] ?? key;
+ }
+
+ return key;
+};
+
+export const FieldMultiSelect = ({ name, param }: FlexibleFormElementProps) =>
{
+ const [selectedOptions, setSelectedOptions] = useState(
+Array.isArray(param.value)
+ ? (param.value as Array).map((value) => ({
+ label: labelLookup(value, param.schema.values_display),
+ value,
+}))
+ : [],
+ );
+
+ return (
+http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import type { ParamSchema } from "src/queries/useDagParams";
+
+import type { FlexibleFormElementProps } from ".";
+import { Hidden } from "./Hidden";
+import { NormalRow } from "./NormalRow";
+
+const isHidden = (fieldSchema: ParamSchema) => Boolean(fieldSchema.const);
+
+/** Generates a form row */
+export const Row = ({ key, name, param }: FlexibleFormElementProps) =>
+ isHidden(param.schema) ? (
+
+ ) : (
+
+ );
Review Comment:
```suggestion
export const Row = ({ name, param }: FlexibleFormElementProps) =>
isHidden(param.schema) ? (
) : (
);
```
##
airflow/ui/src/components/FlexibleForm/SelectElement.tsx:
##
@@ -0,0 +1,120 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import type { ParamSchema, ParamSpec } from "src/queries/useDagParams";
+
+import type { FlexibleFormElementProps } from ".";
+import { FieldAdvancedArray } from "./FieldAdvancedArray";
+import { FieldBool } from "./FieldBool";
+import { FieldDate } from "./FieldDate";
+import { FieldDateTime } from "./FieldDateTime";
+import { FieldDropdown } from "./FieldDropdown";
+import { FieldMultiSelect } from "./FieldMultiSelect";
+import { FieldMultilineText } from "./FieldMultilineText";
+import { FieldNumber } from "./FieldNumber";
+import { FieldObject } from "./FieldObject";
+import { FieldString } from "./FieldString";
+import { FieldStringArray } from "./FieldStringArray";
+import { FieldTime } from "./FieldTime";
+
+const inferType = (param: ParamSpec) => {
+ if (Boolean(param.schema.type))
Re: [PR] Migrate trigger form params to new UI [airflow]
jscheffl commented on PR #45270: URL: https://github.com/apache/airflow/pull/45270#issuecomment-2585742519 Hi @bbovenzi I aligned in Slack with @shubhamraj-git that he would take the "wire-up" of reset-button and sync of form fields <-> Advanced Options JSON in a follow up PR the next days. So this PR is mainly the general component start and layout, function coming in the next PR. Would it be OK frmo point of code/layout/structure mo merge? Then would need your review/approval. -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Migrate trigger form params to new UI [airflow]
jscheffl commented on PR #45270: URL: https://github.com/apache/airflow/pull/45270#issuecomment-2585391171 Had a few "sleeps" over the discussion I had in Slack with @shubhamraj-git about Accordion.. and after Pierre also assigned it to the Note for Clear tasks... I am somewhat open and ajusted... as forms can get long. All is not (last commit) in one accordion group. Looks like this and parts are nicely folding:  Changing to Advanced options:  WDYT? -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Migrate trigger form params to new UI [airflow]
jscheffl commented on code in PR #45270:
URL: https://github.com/apache/airflow/pull/45270#discussion_r1907918804
##
airflow/ui/src/components/FlexibleForm/FlexibleFormFieldString.tsx:
##
@@ -0,0 +1,45 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Input } from "@chakra-ui/react";
+
+import type { FlexibleFormElementProps } from ".";
+
+export const FlexibleFormFieldString = ({ name, param }:
FlexibleFormElementProps) => (
+ <>
+
Re: [PR] Migrate trigger form params to new UI [airflow]
jscheffl commented on code in PR #45270:
URL: https://github.com/apache/airflow/pull/45270#discussion_r1907918499
##
airflow/ui/src/components/FlexibleForm/FlexibleFormFieldObject.tsx:
##
@@ -0,0 +1,52 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { json } from "@codemirror/lang-json";
+import { githubLight, githubDark } from "@uiw/codemirror-themes-all";
+import CodeMirror from "@uiw/react-codemirror";
+
+import { useColorMode } from "src/context/colorMode";
+
+import type { FlexibleFormElementProps } from ".";
+
+export const FlexibleFormFieldObject = ({ name, param }:
FlexibleFormElementProps) => {
+ const { colorMode } = useColorMode();
+
+ return (
+
Re: [PR] Migrate trigger form params to new UI [airflow]
bbovenzi commented on code in PR #45270:
URL: https://github.com/apache/airflow/pull/45270#discussion_r1905904051
##
airflow/ui/src/components/FlexibleForm/FlexibleFormFieldObject.tsx:
##
@@ -0,0 +1,52 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { json } from "@codemirror/lang-json";
+import { githubLight, githubDark } from "@uiw/codemirror-themes-all";
+import CodeMirror from "@uiw/react-codemirror";
+
+import { useColorMode } from "src/context/colorMode";
+
+import type { FlexibleFormElementProps } from ".";
+
+export const FlexibleFormFieldObject = ({ name, param }:
FlexibleFormElementProps) => {
+ const { colorMode } = useColorMode();
+
+ return (
+
Re: [PR] Migrate trigger form params to new UI [airflow]
shubhamraj-git commented on code in PR #45270:
URL: https://github.com/apache/airflow/pull/45270#discussion_r1903316647
##
airflow/ui/src/components/FlexibleForm/FlexibleFormFieldDate.tsx:
##
@@ -0,0 +1,37 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Input } from "@chakra-ui/react";
+
+import type { ParamSchema } from "src/queries/useDagParams";
+
+import type { FlexibleFormElementProps } from ".";
+
+export const isFieldDate = (fieldType: string, fieldSchema: ParamSchema) =>
+ fieldType === "string" && fieldSchema.format === "date";
+
+export const FlexibleFormFieldDate = ({ name, param }:
FlexibleFormElementProps) => (
+
Re: [PR] Migrate trigger form params to new UI [airflow]
shubhamraj-git commented on code in PR #45270:
URL: https://github.com/apache/airflow/pull/45270#discussion_r1903283559
##
airflow/ui/src/components/FlexibleForm/FlexibleFormFieldString.tsx:
##
@@ -0,0 +1,45 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Input } from "@chakra-ui/react";
+
+import type { FlexibleFormElementProps } from ".";
+
+export const FlexibleFormFieldString = ({ name, param }:
FlexibleFormElementProps) => (
+ <>
+
Re: [PR] Migrate trigger form params to new UI [airflow]
jscheffl commented on code in PR #45270:
URL: https://github.com/apache/airflow/pull/45270#discussion_r1903120502
##
airflow/ui/src/components/FlexibleForm/FlexibleFormFieldAdvancedArray.tsx:
##
@@ -0,0 +1,56 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { json } from "@codemirror/lang-json";
+import { githubLight, githubDark } from "@uiw/codemirror-themes-all";
+import CodeMirror from "@uiw/react-codemirror";
+
+import { useColorMode } from "src/context/colorMode";
+import type { ParamSchema } from "src/queries/useDagParams";
+
+import type { FlexibleFormElementProps } from ".";
+
+export const isFieldAdvancedArray = (fieldType: string, fieldSchema:
ParamSchema) =>
+ fieldType === "array" && fieldSchema.items?.type !== "string";
+
Review Comment:
Oh, yeah - also saw this. Now I re-located all selector functions... I
thought of keeping the logic of field and selection together... for the future
to have maybe a plugin mechanism... but yeah can be reworked in future as well.
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Migrate trigger form params to new UI [airflow]
jscheffl commented on code in PR #45270:
URL: https://github.com/apache/airflow/pull/45270#discussion_r1903120351
##
airflow/ui/src/components/FlexibleForm/FlexibleFormFieldDate.tsx:
##
@@ -0,0 +1,37 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Input } from "@chakra-ui/react";
+
+import type { ParamSchema } from "src/queries/useDagParams";
+
+import type { FlexibleFormElementProps } from ".";
+
+export const isFieldDate = (fieldType: string, fieldSchema: ParamSchema) =>
+ fieldType === "string" && fieldSchema.format === "date";
+
+export const FlexibleFormFieldDate = ({ name, param }:
FlexibleFormElementProps) => (
+ http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import type { FlexibleFormElementProps } from ".";
+import { Switch } from "../ui";
+
+export const isFieldBool = (fieldType: string) => fieldType === "boolean";
+
+export const FlexibleFormFieldBool = ({ name, param }:
FlexibleFormElementProps) => (
+
Re: [PR] Migrate trigger form params to new UI [airflow]
shubhamraj-git commented on code in PR #45270:
URL: https://github.com/apache/airflow/pull/45270#discussion_r1901963147
##
airflow/ui/src/components/FlexibleForm/FlexibleFormFieldBool.tsx:
##
@@ -0,0 +1,31 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import type { FlexibleFormElementProps } from ".";
+import { Switch } from "../ui";
+
+export const isFieldBool = (fieldType: string) => fieldType === "boolean";
+
+export const FlexibleFormFieldBool = ({ name, param }:
FlexibleFormElementProps) => (
+ http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Input } from "@chakra-ui/react";
+
+import type { ParamSchema } from "src/queries/useDagParams";
+
+import type { FlexibleFormElementProps } from ".";
+
+export const isFieldDate = (fieldType: string, fieldSchema: ParamSchema) =>
+ fieldType === "string" && fieldSchema.format === "date";
+
+export const FlexibleFormFieldDate = ({ name, param }:
FlexibleFormElementProps) => (
+ http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { json } from "@codemirror/lang-json";
+import { githubLight, githubDark } from "@uiw/codemirror-themes-all";
+import CodeMirror from "@uiw/react-codemirror";
+
+import { useColorMode } from "src/context/colorMode";
+import type { ParamSchema } from "src/queries/useDagParams";
+
+import type { FlexibleFormElementProps } from ".";
+
+export const isFieldAdvancedArray = (fieldType: string, fieldSchema:
ParamSchema) =>
+ fieldType === "array" && fieldSchema.items?.type !== "string";
+
Review Comment:
I see many utility func in each component.
Can we have a separate file to have all that one ? Maybe
`FlexibleFormUtility` , no hard suggestions.
This will remove the warnings too..
```
Fast refresh only works when a file only exports components. Use a new file
to share constants or functions between components.
```
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Migrate trigger form params to new UI [airflow]
jscheffl commented on PR #45270: URL: https://github.com/apache/airflow/pull/45270#issuecomment-2567986116 > Thanks @jscheffl for working on this. Had the first look. Looks great. The base is almost ready. > > As mentioned in TODO, we will support the accordion and not flat. > > Some points which i noticed, just writing it down incase we don't miss. > > 1. There is no live changes to conf json onBlur > > 2. Should have a validation for integer inputs. > > 3. Dropdown not working for select values > > 4. we should have a time picker. > > 5. Length check not working >~6. JSON inputs should have a code view? (Just a opinion)~ > > 6. List should have multi lines > > > Overall this is going in right direction. > > Should we have inbuilt scroll for the dynamic form? since the modal seems very long. > > EDIT: Just saw a todo for point 6 (json code view) and most of the above issues as todo. (1) I recommend to make into a separate PR - as also discussed 1:1 (2,3,5,6) fixed. (4) would need a component fix in react for Chrome... and a workaround for Firefox. Firfox time picking is "just broken" - would make this to a separate PR as well. So... for the Layout part I'd say... ready for review! -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Migrate trigger form params to new UI [airflow]
shubhamraj-git commented on PR #45270: URL: https://github.com/apache/airflow/pull/45270#issuecomment-2565833235 Thanks @jscheffl for working on this. Had the first look. Its going good. As mentioned in TODO, we will support the accordion and not flat. Some points which i noticed, just writing it down incase we don't miss. 1. There is no live changes to conf json onBlur 2. Should have a validation for integer inputs. 3. Dropdown not working for select values 4. we should have a time picker. 5. Length check not working 6. JSON inputs should have a code view? (Just a opinion) 7. List should have multi lines Overall this is going in right direction. Should we have inbuilt scroll for the dynamic form? since the modal seems very long. -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
