Copilot commented on code in PR #3323: URL: https://github.com/apache/apisix-dashboard/pull/3323#discussion_r2921771216
########## src/components/Header/ThemeToggle.tsx: ########## @@ -0,0 +1,76 @@ +/** + * 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 { SegmentedControl, useMantineColorScheme } from '@mantine/core'; +import { useCallback, useEffect, useRef } from 'react'; + +import IconDarkMode from '~icons/material-symbols/dark-mode'; +import IconDesktop from '~icons/material-symbols/desktop-windows'; +import IconLightMode from '~icons/material-symbols/light-mode'; + +const TRANSITION_DURATION_MS = 250; Review Comment: `TRANSITION_DURATION_MS` (250ms) does not match the CSS transition duration (0.2s) used by `.theme-transitioning`. This makes the cleanup timing harder to reason about and can leave the class applied longer than the actual transition. Consider deriving both from a single source (e.g., a CSS variable) or making the values consistent. ```suggestion const TRANSITION_DURATION_MS = 200; ``` ########## src/styles/global.css: ########## @@ -3,6 +3,12 @@ border-radius: var(--mantine-radius-sm); } +body.theme-transitioning, +body.theme-transitioning * { + transition: background-color 0.2s ease, color 0.2s ease, + border-color 0.2s ease; +} + Review Comment: This rule sets `transition` on `body.theme-transitioning *`, which overrides any existing `transition` definitions while the class is present. That can unintentionally break component animations (e.g., opacity/transform transitions) if they occur during a theme toggle. Consider scoping the rule more narrowly or using a mechanism that doesn't clobber other transitions. ```suggestion body.theme-transitioning { transition: background-color 0.2s ease, color 0.2s ease, border-color 0.2s ease; } ``` ########## src/components/Header/ThemeToggle.tsx: ########## @@ -0,0 +1,76 @@ +/** + * 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 { SegmentedControl, useMantineColorScheme } from '@mantine/core'; +import { useCallback, useEffect, useRef } from 'react'; + +import IconDarkMode from '~icons/material-symbols/dark-mode'; +import IconDesktop from '~icons/material-symbols/desktop-windows'; +import IconLightMode from '~icons/material-symbols/light-mode'; + +const TRANSITION_DURATION_MS = 250; + +const iconStyle = { width: 14, height: 14 } as const; + +const segmentData = [ + { value: 'light', label: <IconLightMode style={iconStyle} /> }, + { value: 'dark', label: <IconDarkMode style={iconStyle} /> }, + { value: 'auto', label: <IconDesktop style={iconStyle} /> }, +]; + +export const ThemeToggle = () => { + const { colorScheme, setColorScheme } = useMantineColorScheme({ + keepTransitions: true, + }); + const timerRef = useRef<ReturnType<typeof setTimeout>>(null); Review Comment: `timerRef` is initialized with `null` but the ref type does not allow `null` under `tsconfig` strict mode, which will fail type-checking. Make the ref type nullable (or initialize with a timeout handle) so `timerRef.current = null` is also type-safe. ```suggestion const timerRef = useRef<ReturnType<typeof setTimeout> | null>(null); ``` ########## index.html: ########## @@ -5,6 +5,16 @@ <link rel="icon" type="image/svg+xml" href="/src/assets/apisix-logo.svg" /> <meta name="viewport" content="width=device-width, initial-scale=1.0" /> <title>Apache APISIX Dashboard</title> + <script> + try { + var scheme = localStorage.getItem('mantine-color-scheme-value'); + if (scheme === 'dark' || scheme === 'light') { + document.documentElement.setAttribute('data-mantine-color-scheme', scheme); + } else if (window.matchMedia && window.matchMedia('(prefers-color-scheme: dark)').matches) { + document.documentElement.setAttribute('data-mantine-color-scheme', 'dark'); Review Comment: The initial color-scheme bootstrap script never sets `data-mantine-color-scheme` to `'light'` when the stored value is `'auto'` (or missing) and the OS prefers light. That can leave the attribute unset on first paint, which may cause inconsistent initial theming depending on how Mantine styles are applied. Handle the `'auto'` case explicitly and/or set `'light'` in the non-dark branch to ensure deterministic initial state. ```suggestion } else { var prefersDark = window.matchMedia && window.matchMedia('(prefers-color-scheme: dark)').matches; document.documentElement.setAttribute( 'data-mantine-color-scheme', prefersDark ? 'dark' : 'light' ); ``` -- 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]
