You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "pierrejeambrun (via GitHub)" <gi...@apache.org> on 2024/12/19 22:34:35 UTC
[PR] Unify Trigger and Clear Action button [airflow]
pierrejeambrun opened a new pull request, #45097:
URL: https://github.com/apache/airflow/pull/45097
The goal is:
- Unify the action button behavior for clear / trigger and any further actions.
- Support a `withText=true` mode for a full button
- Support a `withText=false` mode for a small, compact action style button with tooltip
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Unify Trigger and Clear Action button [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #45097:
URL: https://github.com/apache/airflow/pull/45097#discussion_r1893226561
##########
airflow/ui/src/components/ui/ActionButton.tsx:
##########
@@ -0,0 +1,61 @@
+/*!
+ * 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 ButtonProps, IconButton } from "@chakra-ui/react";
+import type { FC, ReactElement } from "react";
+
+import { Button, Tooltip } from "src/components/ui";
+
+type Props = {
+ readonly actionName: string;
+ readonly colorPalette?: string;
+ readonly icon: ReactElement;
+ readonly onClick: () => void;
+ readonly text: string;
+ readonly variant?: string;
+ readonly withText?: boolean;
+} & ButtonProps;
+
+const ActionButton = ({
+ actionName,
+ colorPalette,
+ icon,
+ onClick,
+ text,
+ variant = "outline",
+ withText = true,
+}: Props) => {
+ const ButtonComponent: FC<ButtonProps> = withText ? Button : IconButton;
+
+ return (
+ <Tooltip content={actionName} disabled={Boolean(withText)}>
+ <ButtonComponent
+ aria-label={actionName}
+ colorPalette={withText ? colorPalette : "blue"}
+ onClick={onClick}
+ size={withText ? "md" : "sm"}
+ variant={withText ? variant : "ghost"}
Review Comment:
The final `TriggerDAGButton` and `ClearRunButton` props are clean and easy to use though. Making sure those actions have consistant display over different places, it's just that intermediate layer that I don't really like.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Unify Trigger and Clear Action button [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun merged PR #45097:
URL: https://github.com/apache/airflow/pull/45097
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Unify Trigger and Clear Action button [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #45097:
URL: https://github.com/apache/airflow/pull/45097#discussion_r1894102328
##########
airflow/ui/src/components/ui/ActionButton.tsx:
##########
@@ -0,0 +1,61 @@
+/*!
+ * 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 ButtonProps, IconButton } from "@chakra-ui/react";
+import type { FC, ReactElement } from "react";
+
+import { Button, Tooltip } from "src/components/ui";
+
+type Props = {
+ readonly actionName: string;
+ readonly colorPalette?: string;
+ readonly icon: ReactElement;
+ readonly onClick: () => void;
+ readonly text: string;
+ readonly variant?: string;
+ readonly withText?: boolean;
+} & ButtonProps;
+
+const ActionButton = ({
+ actionName,
+ colorPalette,
+ icon,
+ onClick,
+ text,
+ variant = "outline",
+ withText = true,
+}: Props) => {
+ const ButtonComponent: FC<ButtonProps> = withText ? Button : IconButton;
+
+ return (
+ <Tooltip content={actionName} disabled={Boolean(withText)}>
+ <ButtonComponent
+ aria-label={actionName}
+ colorPalette={withText ? colorPalette : "blue"}
+ onClick={onClick}
+ size={withText ? "md" : "sm"}
+ variant={withText ? variant : "ghost"}
Review Comment:
That's not really critical. Maybe someone will find a more flexible way. We don't really need it for now.
Moving forward.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Unify Trigger and Clear Action button [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #45097:
URL: https://github.com/apache/airflow/pull/45097#discussion_r1893226561
##########
airflow/ui/src/components/ui/ActionButton.tsx:
##########
@@ -0,0 +1,61 @@
+/*!
+ * 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 ButtonProps, IconButton } from "@chakra-ui/react";
+import type { FC, ReactElement } from "react";
+
+import { Button, Tooltip } from "src/components/ui";
+
+type Props = {
+ readonly actionName: string;
+ readonly colorPalette?: string;
+ readonly icon: ReactElement;
+ readonly onClick: () => void;
+ readonly text: string;
+ readonly variant?: string;
+ readonly withText?: boolean;
+} & ButtonProps;
+
+const ActionButton = ({
+ actionName,
+ colorPalette,
+ icon,
+ onClick,
+ text,
+ variant = "outline",
+ withText = true,
+}: Props) => {
+ const ButtonComponent: FC<ButtonProps> = withText ? Button : IconButton;
+
+ return (
+ <Tooltip content={actionName} disabled={Boolean(withText)}>
+ <ButtonComponent
+ aria-label={actionName}
+ colorPalette={withText ? colorPalette : "blue"}
+ onClick={onClick}
+ size={withText ? "md" : "sm"}
+ variant={withText ? variant : "ghost"}
Review Comment:
The final `TriggerDAGButton` and `ClearRunButton` props are clean and easy to use. Making sure those actions have consistant display over different places, it's just that intermediate layer that I don't really like.
##########
airflow/ui/src/components/ui/ActionButton.tsx:
##########
@@ -0,0 +1,61 @@
+/*!
+ * 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 ButtonProps, IconButton } from "@chakra-ui/react";
+import type { FC, ReactElement } from "react";
+
+import { Button, Tooltip } from "src/components/ui";
+
+type Props = {
+ readonly actionName: string;
+ readonly colorPalette?: string;
+ readonly icon: ReactElement;
+ readonly onClick: () => void;
+ readonly text: string;
+ readonly variant?: string;
+ readonly withText?: boolean;
+} & ButtonProps;
+
+const ActionButton = ({
+ actionName,
+ colorPalette,
+ icon,
+ onClick,
+ text,
+ variant = "outline",
+ withText = true,
+}: Props) => {
+ const ButtonComponent: FC<ButtonProps> = withText ? Button : IconButton;
+
+ return (
+ <Tooltip content={actionName} disabled={Boolean(withText)}>
+ <ButtonComponent
+ aria-label={actionName}
+ colorPalette={withText ? colorPalette : "blue"}
+ onClick={onClick}
+ size={withText ? "md" : "sm"}
+ variant={withText ? variant : "ghost"}
Review Comment:
`variant` and `colorPalette` (or any other custom styling for 'withText') I don't like but couldn't find an elegant way.
Meaning that it's not super easy to use an `<ActionButton />` and customize style for `withText` and `compact` (withText=false) version distinctly.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Unify Trigger and Clear Action button [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #45097:
URL: https://github.com/apache/airflow/pull/45097#discussion_r1894104791
##########
airflow/ui/src/components/ui/ActionButton.tsx:
##########
@@ -0,0 +1,61 @@
+/*!
+ * 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 ButtonProps, IconButton } from "@chakra-ui/react";
+import type { FC, ReactElement } from "react";
+
+import { Button, Tooltip } from "src/components/ui";
+
+type Props = {
+ readonly actionName: string;
+ readonly colorPalette?: string;
+ readonly icon: ReactElement;
+ readonly onClick: () => void;
+ readonly text: string;
+ readonly variant?: string;
+ readonly withText?: boolean;
+} & ButtonProps;
+
+const ActionButton = ({
+ actionName,
+ colorPalette,
+ icon,
+ onClick,
+ text,
+ variant = "outline",
+ withText = true,
+}: Props) => {
+ const ButtonComponent: FC<ButtonProps> = withText ? Button : IconButton;
+
+ return (
+ <Tooltip content={actionName} disabled={Boolean(withText)}>
+ <ButtonComponent
+ aria-label={actionName}
+ colorPalette={withText ? colorPalette : "blue"}
+ onClick={onClick}
+ size={withText ? "md" : "sm"}
+ variant={withText ? variant : "ghost"}
Review Comment:
If things didin't change there's no inheritance in react but Composition. I tried thinking about that but I didn't find an ideal approach.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Unify Trigger and Clear Action button [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #45097:
URL: https://github.com/apache/airflow/pull/45097#discussion_r1893228020
##########
airflow/ui/src/components/ui/ActionButton.tsx:
##########
@@ -0,0 +1,61 @@
+/*!
+ * 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 ButtonProps, IconButton } from "@chakra-ui/react";
+import type { FC, ReactElement } from "react";
+
+import { Button, Tooltip } from "src/components/ui";
+
+type Props = {
+ readonly actionName: string;
+ readonly colorPalette?: string;
+ readonly icon: ReactElement;
+ readonly onClick: () => void;
+ readonly text: string;
+ readonly variant?: string;
+ readonly withText?: boolean;
+} & ButtonProps;
+
+const ActionButton = ({
+ actionName,
+ colorPalette,
+ icon,
+ onClick,
+ text,
+ variant = "outline",
+ withText = true,
+}: Props) => {
+ const ButtonComponent: FC<ButtonProps> = withText ? Button : IconButton;
+
+ return (
+ <Tooltip content={actionName} disabled={Boolean(withText)}>
+ <ButtonComponent
+ aria-label={actionName}
+ colorPalette={withText ? colorPalette : "blue"}
+ onClick={onClick}
+ size={withText ? "md" : "sm"}
+ variant={withText ? variant : "ghost"}
Review Comment:
I am not a pro in React... is some kind of inheritance also possible (compared to use an abstract component in another component as middle layer)?
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org