ryanahamilton commented on code in PR #57455:
URL: https://github.com/apache/airflow/pull/57455#discussion_r2471297679
##########
airflow-core/src/airflow/ui/src/layouts/Nav/AdminButton.tsx:
##########
@@ -79,7 +79,7 @@ export const AdminButton = ({
return (
<Menu.Root positioning={{ placement: "right" }}>
<Menu.Trigger asChild>
- <NavButton icon={<FiSettings size={28} />}
title={translate("nav.admin")} />
Review Comment:
`NavButton`'s internal implementation of the `icon` prop was refactored so
the size doesn't have to be defined at every implementation as it currently is.
##########
airflow-core/src/airflow/ui/src/layouts/Nav/NavButton.tsx:
##########
@@ -16,62 +16,104 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { Box, Button, Link, type ButtonProps } from "@chakra-ui/react";
-import type { ReactElement } from "react";
-import { NavLink } from "react-router-dom";
+import { Box, type BoxProps, Button, Icon, type IconProps, Link, type
ButtonProps } from "@chakra-ui/react";
+import { useMemo, type ForwardRefExoticComponent, type RefAttributes } from
"react";
+import { IconType } from "react-icons";
+import { NavLink, useMatch } from "react-router-dom";
-const styles = {
- _active: {
- bg: "brand.emphasized",
- },
- // Fix inverted hover and active colors
- _hover: {
- bg: "brand.emphasized", // Even darker for better light mode contrast
- },
- alignItems: "center",
- borderRadius: "none",
- colorPalette: "brand",
- flexDir: "column",
- height: 20,
- variant: "ghost",
- whiteSpace: "wrap",
- width: 20,
-} satisfies ButtonProps;
+const commonLabelProps: BoxProps = {
+ fontSize: "2xs",
+ overflow: "hidden",
+ textAlign: "center",
+ textOverflow: "ellipsis",
+ whiteSpace: "nowrap",
+ width: "full",
+};
type NavButtonProps = {
- readonly icon: ReactElement;
+ readonly icon: IconType | ForwardRefExoticComponent<IconProps &
RefAttributes<SVGSVGElement>>;
readonly isExternal?: boolean;
- readonly title?: string;
+ readonly title: string;
readonly to?: string;
} & ButtonProps;
-export const NavButton = ({ icon, isExternal = false, title, to, ...rest }:
NavButtonProps) =>
- to === undefined ? (
- <Button {...styles} {...rest}>
- <Box alignSelf="center">{icon}</Box>
- <Box fontSize="xs">{title}</Box>
- </Button>
- ) : isExternal ? (
Review Comment:
Refactored this without using a nested ternary to improve the legibility of
the file.
##########
airflow-core/src/airflow/ui/src/layouts/Nav/UserSettingsButton.tsx:
##########
@@ -55,89 +55,88 @@ type ColorMode = (typeof COLOR_MODES)[keyof typeof
COLOR_MODES];
export const UserSettingsButton = ({ externalViews }: { readonly
externalViews: Array<NavItemResponse> }) => {
const { i18n, t: translate } = useTranslation();
const { selectedTheme, setColorMode } = useColorMode();
+
+ const colorModeOptions = [
+ {
+ icon: FiSun,
+ label: translate("appearance.lightMode"),
+ value: COLOR_MODES.LIGHT,
+ },
+ {
+ icon: FiMoon,
+ label: translate("appearance.darkMode"),
+ value: COLOR_MODES.DARK,
+ },
+ {
+ icon: FiMonitor,
+ label: translate("appearance.systemMode"),
+ value: COLOR_MODES.SYSTEM,
+ },
+ ];
+
const { onClose: onCloseTimezone, onOpen: onOpenTimezone, open:
isOpenTimezone } = useDisclosure();
const { onClose: onCloseLogout, onOpen: onOpenLogout, open: isOpenLogout } =
useDisclosure();
const { onClose: onCloseLanguage, onOpen: onOpenLanguage, open:
isOpenLanguage } = useDisclosure();
+
const [dagView, setDagView] = useLocalStorage<"graph" |
"grid">("default_dag_view", "grid");
const theme = selectedTheme ?? COLOR_MODES.SYSTEM;
const isRTL = i18n.dir() === "rtl";
return (
- <Menu.Root positioning={{ placement: "right" }}>
- <Menu.Trigger asChild>
- <NavButton icon={<FiUser size={28} />} title={translate("user")} />
- </Menu.Trigger>
- <Menu.Content>
- <Menu.Item onClick={onOpenLanguage} value="language">
- <FiGlobe size={20} style={{ marginRight: "8px" }} />
- {translate("selectLanguage")}
- </Menu.Item>
- <Menu.Root>
- <Menu.TriggerItem>
- <FiEye size={20} style={{ marginRight: "8px" }} />
- {translate("appearance.appearance")}
- {isRTL ? (
- <FiChevronLeft size={20} style={{ marginRight: "auto" }} />
- ) : (
- <FiChevronRight size={20} style={{ marginLeft: "auto" }} />
- )}
- </Menu.TriggerItem>
- <Menu.Content>
- <Menu.RadioItemGroup
- onValueChange={(element) => setColorMode(element.value as
ColorMode)}
- value={theme}
- >
- <Menu.RadioItem value={COLOR_MODES.LIGHT}>
- <FiSun size={20} style={{ marginRight: "8px" }} />
- {translate("appearance.lightMode")}
- <Menu.ItemIndicator />
- </Menu.RadioItem>
- <Menu.RadioItem value={COLOR_MODES.DARK}>
- <FiMoon size={20} style={{ marginRight: "8px" }} />
- {translate("appearance.darkMode")}
- <Menu.ItemIndicator />
- </Menu.RadioItem>
- <Menu.RadioItem value={COLOR_MODES.SYSTEM}>
- <FiMonitor size={20} style={{ marginRight: "8px" }} />
- {translate("appearance.systemMode")}
- <Menu.ItemIndicator />
- </Menu.RadioItem>
- </Menu.RadioItemGroup>
- </Menu.Content>
- </Menu.Root>
- <Menu.Item
- onClick={() => (dagView === "grid" ? setDagView("graph") :
setDagView("grid"))}
- value={dagView}
- >
- {dagView === "grid" ? (
- <>
- <MdOutlineAccountTree size={20} style={{ marginRight: "8px" }} />
- {translate("defaultToGraphView")}
- </>
- ) : (
- <>
- <FiGrid size={20} style={{ marginRight: "8px" }} />
- {translate("defaultToGridView")}
- </>
- )}
- </Menu.Item>
- <TimezoneMenuItem onOpen={onOpenTimezone} />
- {externalViews.map((view) => (
- <PluginMenuItem {...view} key={view.name} />
- ))}
- <Menu.Item onClick={onOpenLogout} value="logout">
- <FiLogOut
- size={20}
- style={{ marginRight: "8px", transform: isRTL ? "rotate(180deg)" :
undefined }}
- />
- {translate("logout")}
- </Menu.Item>
- </Menu.Content>
+ <>
+ <Menu.Root positioning={{ placement: "right" }}>
+ <Menu.Trigger asChild>
+ <NavButton icon={FiUser} title={translate("user")} />
+ </Menu.Trigger>
+ <Menu.Content>
+ <Menu.Item onClick={onOpenLanguage} value="language">
+ <Icon as={FiGlobe} boxSize={4} />
+ <Box flex="1">{translate("selectLanguage")}</Box>
+ </Menu.Item>
+ <Menu.Root>
+ <Menu.TriggerItem>
+ <Icon as={FiEye} boxSize={4} />
+ <Box flex="1">{translate("appearance.appearance")}</Box>
+ <Icon as={isRTL ? FiChevronLeft : FiChevronRight} boxSize={4}
color="fg.muted" />
+ </Menu.TriggerItem>
+ <Menu.Content>
+ <Menu.RadioItemGroup
+ onValueChange={(element) => setColorMode(element.value as
ColorMode)}
+ value={theme}
+ >
+ {colorModeOptions.map(({ icon, label, value }) => (
+ <Menu.RadioItem key={value} value={value}>
+ <Icon as={icon} boxSize={4} />
+ <Box flex="1">{label}</Box>
+ <Menu.ItemIndicator color="fg.muted" />
+ </Menu.RadioItem>
+ ))}
+ </Menu.RadioItemGroup>
+ </Menu.Content>
+ </Menu.Root>
+ <Menu.Item
+ onClick={() => (dagView === "grid" ? setDagView("graph") :
setDagView("grid"))}
+ value={dagView}
+ >
+ <Icon as={dagView === "grid" ? MdOutlineAccountTree : FiGrid}
boxSize={4} />
+ <Box flex="1">{dagView === "grid" ?
translate("defaultToGraphView") : translate("defaultToGridView")}</Box>
+ </Menu.Item>
+ <TimezoneMenuItem onOpen={onOpenTimezone} />
+ {externalViews.map((view) => (
+ <PluginMenuItem {...view} key={view.name} />
+ ))}
+ <Menu.Separator />
+ <Menu.Item onClick={onOpenLogout} value="logout">
+ <Icon as={FiLogOut} boxSize={4} transform={isRTL ?
"rotate(180deg)" : undefined} />
+ <Box flex="1">{translate("logout")}</Box>
+ </Menu.Item>
+ </Menu.Content>
+ </Menu.Root>
<LanguageModal isOpen={isOpenLanguage} onClose={onCloseLanguage} />
<TimezoneModal isOpen={isOpenTimezone} onClose={onCloseTimezone} />
<LogoutModal isOpen={isOpenLogout} onClose={onCloseLogout} />
- </Menu.Root>
+ </>
Review Comment:
Added the fragment wrapper so that the Modal components didn't need to be
nested within `Menu.Root`.
--
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]
