pierrejeambrun commented on code in PR #56344:
URL: https://github.com/apache/airflow/pull/56344#discussion_r2402389554
##########
airflow-core/src/airflow/ui/src/components/DisplayMarkdownButton.tsx:
##########
@@ -20,9 +20,12 @@ import { Box, Heading, VStack } from "@chakra-ui/react";
import { type ReactElement, useState } from "react";
import { Button, Dialog } from "src/components/ui";
+import { ResizableWrapper } from "src/components/ui/ResizableWrapper";
import ReactMarkdown from "./ReactMarkdown";
+const STORAGE_KEY = "airflow-markdown-dialog-size";
Review Comment:
All dag documentation + ti documentation will share the same setting / key.
I'm fine with that, I just wanted to confirm
##########
airflow-core/src/airflow/ui/src/utils/usePersistentResizableState.ts:
##########
@@ -0,0 +1,69 @@
+/*!
+ * 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 { useCallback, useState } from "react";
+
+type Size = { height: number; width: number };
+
+const getInitialSize = (key: string, defaultSize: Size): Size => {
+ try {
+ const item = localStorage.getItem(key);
+
+ if (item === null) {
+ return defaultSize;
+ }
+
+ const parsed: unknown = JSON.parse(item);
+
+ if (
+ typeof parsed === "object" &&
+ parsed !== null &&
+ "width" in parsed &&
+ "height" in parsed &&
+ typeof parsed.width === "number" &&
+ typeof parsed.height === "number"
+ ) {
+ return { height: parsed.height, width: parsed.width };
+ }
+ } catch {
+ // Ignore parsing errors
+ }
+
+ return defaultSize;
+};
+
+export const usePersistentResizableState = (storageKey: string, defaultSize:
Size) => {
+ const [size, setSize] = useState(() => getInitialSize(storageKey,
defaultSize));
+
+ const handleResize = useCallback((_event: React.SyntheticEvent, { size:
newSize }: { size: Size }) => {
+ setSize(newSize);
+ }, []);
+
+ const handleResizeStop = useCallback(
+ (_event: React.SyntheticEvent, { size: finalSize }: { size: Size }) => {
+ try {
+ localStorage.setItem(storageKey, JSON.stringify(finalSize));
+ } catch {
+ // Ignore storage errors
+ }
+ },
Review Comment:
How could a `setItem` fail? Why is that 'expected' and silently ignored?
##########
airflow-core/src/airflow/ui/src/components/ui/ResizableWrapper.tsx:
##########
@@ -0,0 +1,88 @@
+/*!
+ * 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 { Box } from "@chakra-ui/react";
+import { forwardRef } from "react";
+import type { ReactNode } from "react";
+import { ResizableBox } from "react-resizable";
+import "react-resizable/css/styles.css";
+
+import { usePersistentResizableState } from
"src/utils/usePersistentResizableState";
+
+const ResizeHandle = forwardRef<HTMLDivElement>((props, ref) => (
+ <Box
+ _before={{
+ background:
+ "linear-gradient(-45deg, transparent 6px, #ccc 6px, #ccc 8px,
transparent 8px, transparent 12px, #ccc 12px, #ccc 14px, transparent 14px)",
+ bottom: 0,
+ content: '""',
+ height: "100%",
+ position: "absolute",
+ right: 0,
+ width: "100%",
+ }}
+ bottom={0}
+ cursor="se-resize"
+ height="20px"
+ position="absolute"
+ ref={ref}
+ right={0}
+ width="20px"
+ {...props}
+ />
Review Comment:
This can be cleaned I think:
```suggestion
<Box
background="linear-gradient(-45deg, transparent 6px, #ccc 6px, #ccc 8px,
transparent 8px, transparent 12px, #ccc 12px, #ccc 14px, transparent 14px)"
backgroundColor="blue"
bottom={0}
content='""'
cursor="se-resize"
height={5}
position="absolute"
ref={ref}
right={0}
width={5}
{...props}
/>
```
##########
airflow-core/src/airflow/ui/src/utils/usePersistentResizableState.ts:
##########
@@ -0,0 +1,69 @@
+/*!
+ * 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 { useCallback, useState } from "react";
+
+type Size = { height: number; width: number };
+
+const getInitialSize = (key: string, defaultSize: Size): Size => {
+ try {
+ const item = localStorage.getItem(key);
+
+ if (item === null) {
+ return defaultSize;
+ }
+
+ const parsed: unknown = JSON.parse(item);
+
+ if (
+ typeof parsed === "object" &&
+ parsed !== null &&
+ "width" in parsed &&
+ "height" in parsed &&
+ typeof parsed.width === "number" &&
+ typeof parsed.height === "number"
+ ) {
+ return { height: parsed.height, width: parsed.width };
+ }
+ } catch {
+ // Ignore parsing errors
+ }
Review Comment:
Why do we need this? Trying to protect against garbage data in the storage
value?
##########
airflow-core/src/airflow/ui/src/components/DisplayMarkdownButton.tsx:
##########
@@ -48,14 +51,24 @@ const DisplayMarkdownButton = ({
open={isDocsOpen}
size="md"
>
- <Dialog.Content backdrop>
- <Dialog.Header bg="info.muted">
- <Heading size="xl">{header}</Heading>
- <Dialog.CloseTrigger closeButtonProps={{ size: "xl" }} />
- </Dialog.Header>
- <Dialog.Body alignItems="flex-start" as={VStack} gap="0">
- <ReactMarkdown>{mdContent}</ReactMarkdown>
- </Dialog.Body>
+ <Dialog.Content
+ backdrop
+ style={{
+ maxHeight: "none",
+ maxWidth: "none",
+ padding: 0,
+ width: "auto",
+ }}
Review Comment:
We don't need to use `style` here. We can inline this, and use Chakra
interface.
##########
airflow-core/src/airflow/ui/src/components/ui/ResizableWrapper.tsx:
##########
@@ -0,0 +1,88 @@
+/*!
+ * 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 { Box } from "@chakra-ui/react";
+import { forwardRef } from "react";
+import type { ReactNode } from "react";
+import { ResizableBox } from "react-resizable";
+import "react-resizable/css/styles.css";
+
+import { usePersistentResizableState } from
"src/utils/usePersistentResizableState";
+
+const ResizeHandle = forwardRef<HTMLDivElement>((props, ref) => (
+ <Box
+ _before={{
+ background:
+ "linear-gradient(-45deg, transparent 6px, #ccc 6px, #ccc 8px,
transparent 8px, transparent 12px, #ccc 12px, #ccc 14px, transparent 14px)",
+ bottom: 0,
+ content: '""',
+ height: "100%",
+ position: "absolute",
+ right: 0,
+ width: "100%",
+ }}
+ bottom={0}
+ cursor="se-resize"
+ height="20px"
+ position="absolute"
+ ref={ref}
+ right={0}
+ width="20px"
+ {...props}
+ />
+));
+
+type ResizableWrapperProps = {
+ readonly children: ReactNode;
+ readonly defaultSize?: { height: number; width: number };
+ readonly maxConstraints?: [number, number];
+ readonly storageKey: string;
+};
+
+const DEFAULT_SIZE = { height: 400, width: 500 };
+const DEFAULT_MAX: [number, number] = [1200, 800];
Review Comment:
Nit
```suggestion
const MAX_SIZE = [1200, 800];
```
##########
airflow-core/src/airflow/ui/src/utils/usePersistentResizableState.ts:
##########
@@ -0,0 +1,69 @@
+/*!
+ * 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 { useCallback, useState } from "react";
+
+type Size = { height: number; width: number };
+
+const getInitialSize = (key: string, defaultSize: Size): Size => {
+ try {
+ const item = localStorage.getItem(key);
+
+ if (item === null) {
+ return defaultSize;
+ }
+
+ const parsed: unknown = JSON.parse(item);
+
+ if (
+ typeof parsed === "object" &&
+ parsed !== null &&
+ "width" in parsed &&
+ "height" in parsed &&
+ typeof parsed.width === "number" &&
+ typeof parsed.height === "number"
+ ) {
+ return { height: parsed.height, width: parsed.width };
+ }
+ } catch {
+ // Ignore parsing errors
+ }
+
+ return defaultSize;
+};
+
+export const usePersistentResizableState = (storageKey: string, defaultSize:
Size) => {
+ const [size, setSize] = useState(() => getInitialSize(storageKey,
defaultSize));
+
+ const handleResize = useCallback((_event: React.SyntheticEvent, { size:
newSize }: { size: Size }) => {
+ setSize(newSize);
+ }, []);
+
+ const handleResizeStop = useCallback(
+ (_event: React.SyntheticEvent, { size: finalSize }: { size: Size }) => {
+ try {
+ localStorage.setItem(storageKey, JSON.stringify(finalSize));
+ } catch {
+ // Ignore storage errors
+ }
+ },
Review Comment:
We could probably use `import { useLocalStorage } from "usehooks-ts";`
instead of bare `localStorage` API. (Maybe this will make things easier.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
