You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "jscheffl (via GitHub)" <gi...@apache.org> on 2025/08/09 22:28:10 UTC
[PR] Fix handling of dark icons for plugins in nav [airflow]
jscheffl opened a new pull request, #54305:
URL: https://github.com/apache/airflow/pull/54305
I noticed that in the nav the new dark mode icons were not applied. When implementing #53563 I saw that dark mode did not consider icons.
This PR fixes the handling also in dark mode.
Light mode:
<img width="317" height="312" alt="image" src="https://github.com/user-attachments/assets/09db67d7-16ad-4cfd-abba-71bb79c91560" />
Dark mode:
<img width="331" height="262" alt="image" src="https://github.com/user-attachments/assets/cb415664-76d9-405d-bd74-3b5032d5be58" />
--
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] Fix handling of dark icons for plugins in nav [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #54305:
URL: https://github.com/apache/airflow/pull/54305#discussion_r2265345945
##########
airflow-core/src/airflow/ui/src/layouts/Nav/PluginMenuItem.tsx:
##########
@@ -23,19 +23,31 @@ import { RiArchiveStackLine } from "react-icons/ri";
import { Link as RouterLink } from "react-router-dom";
import type { ExternalViewResponse } from "openapi/requests/types.gen";
+import { useColorMode } from "src/context/colorMode";
import type { NavItemResponse } from "src/utils/types";
import { NavButton } from "./NavButton";
type Props = { readonly topLevel?: boolean } & NavItemResponse;
-export const PluginMenuItem = ({ icon, name, topLevel = false, url_route: urlRoute, ...rest }: Props) => {
+export const PluginMenuItem = ({
+ icon,
+ // eslint-disable-next-line @typescript-eslint/naming-convention
+ icon_dark_mode,
Review Comment:
Good hint, did not know about this trick! This made it happy! 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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix handling of dark icons for plugins in nav [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl merged PR #54305:
URL: https://github.com/apache/airflow/pull/54305
--
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] Fix handling of dark icons for plugins in nav [airflow]
Posted by "guan404ming (via GitHub)" <gi...@apache.org>.
guan404ming commented on code in PR #54305:
URL: https://github.com/apache/airflow/pull/54305#discussion_r2265105689
##########
airflow-core/src/airflow/ui/src/layouts/Nav/PluginMenuItem.tsx:
##########
@@ -23,19 +23,31 @@ import { RiArchiveStackLine } from "react-icons/ri";
import { Link as RouterLink } from "react-router-dom";
import type { ExternalViewResponse } from "openapi/requests/types.gen";
+import { useColorMode } from "src/context/colorMode";
import type { NavItemResponse } from "src/utils/types";
import { NavButton } from "./NavButton";
type Props = { readonly topLevel?: boolean } & NavItemResponse;
-export const PluginMenuItem = ({ icon, name, topLevel = false, url_route: urlRoute, ...rest }: Props) => {
+export const PluginMenuItem = ({
+ icon,
+ // eslint-disable-next-line @typescript-eslint/naming-convention
+ icon_dark_mode,
+ name,
+ topLevel = false,
+ url_route: urlRoute,
+ ...rest
+}: Props) => {
// Determine if this is an external view or react app based on the presence of href
+ const { colorMode } = useColorMode();
const isExternalView = "href" in rest;
const href = isExternalView ? (rest as ExternalViewResponse).href : undefined;
const pluginIcon =
- typeof icon === "string" ? (
+ colorMode === "dark" && typeof icon_dark_mode === "string" ? (
+ <Image height="1.25rem" mr={topLevel ? 0 : 2} src={icon_dark_mode} width="1.25rem" />
Review Comment:
We could make two icon use the same component which make it easier to maintain!
```js
<Image height="1.25rem" mr={topLevel ? 0 : 2} src={colorMode === "dark" && typeof icon_dark_mode === "string" ? icon_dark_mode : icon} width="1.25rem" />
```
or
```js
const isDarkMode = colorMode === "dark" && typeof icon_dark_mode === "string"
<Image height="1.25rem" mr={topLevel ? 0 : 2} src={isDarkMode ? icon_dark_mode : icon} width="1.25rem" />
```
##########
airflow-core/src/airflow/ui/src/layouts/Nav/PluginMenuItem.tsx:
##########
@@ -23,19 +23,31 @@ import { RiArchiveStackLine } from "react-icons/ri";
import { Link as RouterLink } from "react-router-dom";
import type { ExternalViewResponse } from "openapi/requests/types.gen";
+import { useColorMode } from "src/context/colorMode";
import type { NavItemResponse } from "src/utils/types";
import { NavButton } from "./NavButton";
type Props = { readonly topLevel?: boolean } & NavItemResponse;
-export const PluginMenuItem = ({ icon, name, topLevel = false, url_route: urlRoute, ...rest }: Props) => {
+export const PluginMenuItem = ({
+ icon,
+ // eslint-disable-next-line @typescript-eslint/naming-convention
+ icon_dark_mode,
Review Comment:
I think this might make eslint happy
```suggestion
icon_dark_mode: iconDarkMode,
```
--
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] Fix handling of dark icons for plugins in nav [airflow]
Posted by "guan404ming (via GitHub)" <gi...@apache.org>.
guan404ming commented on code in PR #54305:
URL: https://github.com/apache/airflow/pull/54305#discussion_r2265350672
##########
airflow-core/src/airflow/ui/src/layouts/Nav/PluginMenuItem.tsx:
##########
@@ -23,20 +23,30 @@ import { RiArchiveStackLine } from "react-icons/ri";
import { Link as RouterLink } from "react-router-dom";
import type { ExternalViewResponse } from "openapi/requests/types.gen";
+import { useColorMode } from "src/context/colorMode";
import type { NavItemResponse } from "src/utils/types";
import { NavButton } from "./NavButton";
type Props = { readonly topLevel?: boolean } & NavItemResponse;
-export const PluginMenuItem = ({ icon, name, topLevel = false, url_route: urlRoute, ...rest }: Props) => {
+export const PluginMenuItem = ({
+ icon,
+ icon_dark_mode: iconDarkMode,
+ name,
+ topLevel = false,
+ url_route: urlRoute,
+ ...rest
+}: Props) => {
// Determine if this is an external view or react app based on the presence of href
+ const { colorMode } = useColorMode();
const isExternalView = "href" in rest;
const href = isExternalView ? (rest as ExternalViewResponse).href : undefined;
+ const someIcon = colorMode === "dark" && typeof iconDarkMode === "string" ? iconDarkMode : icon;
Review Comment:
Just a really tiny nit. Should we name this variable `displayIcon` instead? (Purely a personal preference, feel free to skip if you prefer it as is :)
--
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] Fix handling of dark icons for plugins in nav [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #54305:
URL: https://github.com/apache/airflow/pull/54305#discussion_r2265346316
##########
airflow-core/src/airflow/ui/src/layouts/Nav/PluginMenuItem.tsx:
##########
@@ -23,19 +23,31 @@ import { RiArchiveStackLine } from "react-icons/ri";
import { Link as RouterLink } from "react-router-dom";
import type { ExternalViewResponse } from "openapi/requests/types.gen";
+import { useColorMode } from "src/context/colorMode";
import type { NavItemResponse } from "src/utils/types";
import { NavButton } from "./NavButton";
type Props = { readonly topLevel?: boolean } & NavItemResponse;
-export const PluginMenuItem = ({ icon, name, topLevel = false, url_route: urlRoute, ...rest }: Props) => {
+export const PluginMenuItem = ({
+ icon,
+ // eslint-disable-next-line @typescript-eslint/naming-convention
+ icon_dark_mode,
+ name,
+ topLevel = false,
+ url_route: urlRoute,
+ ...rest
+}: Props) => {
// Determine if this is an external view or react app based on the presence of href
+ const { colorMode } = useColorMode();
const isExternalView = "href" in rest;
const href = isExternalView ? (rest as ExternalViewResponse).href : undefined;
const pluginIcon =
- typeof icon === "string" ? (
+ colorMode === "dark" && typeof icon_dark_mode === "string" ? (
+ <Image height="1.25rem" mr={topLevel ? 0 : 2} src={icon_dark_mode} width="1.25rem" />
Review Comment:
I attempted another way as first we need to select if the icon is anyway in place... so forst depending on color mode the icon is picked with standard as fall back if dark is not defined and then if any icon selected renders the <Image (once).
--
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] Fix handling of dark icons for plugins in nav [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #54305:
URL: https://github.com/apache/airflow/pull/54305#discussion_r2265368651
##########
airflow-core/src/airflow/ui/src/layouts/Nav/PluginMenuItem.tsx:
##########
@@ -23,20 +23,30 @@ import { RiArchiveStackLine } from "react-icons/ri";
import { Link as RouterLink } from "react-router-dom";
import type { ExternalViewResponse } from "openapi/requests/types.gen";
+import { useColorMode } from "src/context/colorMode";
import type { NavItemResponse } from "src/utils/types";
import { NavButton } from "./NavButton";
type Props = { readonly topLevel?: boolean } & NavItemResponse;
-export const PluginMenuItem = ({ icon, name, topLevel = false, url_route: urlRoute, ...rest }: Props) => {
+export const PluginMenuItem = ({
+ icon,
+ icon_dark_mode: iconDarkMode,
+ name,
+ topLevel = false,
+ url_route: urlRoute,
+ ...rest
+}: Props) => {
// Determine if this is an external view or react app based on the presence of href
+ const { colorMode } = useColorMode();
const isExternalView = "href" in rest;
const href = isExternalView ? (rest as ExternalViewResponse).href : undefined;
+ const someIcon = colorMode === "dark" && typeof iconDarkMode === "string" ? iconDarkMode : icon;
Review Comment:
I am not really happy with the name but `displayIcon` is a bit better than `someIcon`.
--
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] Fix handling of dark icons for plugins in nav [airflow]
Posted by "guan404ming (via GitHub)" <gi...@apache.org>.
guan404ming commented on code in PR #54305:
URL: https://github.com/apache/airflow/pull/54305#discussion_r2265105689
##########
airflow-core/src/airflow/ui/src/layouts/Nav/PluginMenuItem.tsx:
##########
@@ -23,19 +23,31 @@ import { RiArchiveStackLine } from "react-icons/ri";
import { Link as RouterLink } from "react-router-dom";
import type { ExternalViewResponse } from "openapi/requests/types.gen";
+import { useColorMode } from "src/context/colorMode";
import type { NavItemResponse } from "src/utils/types";
import { NavButton } from "./NavButton";
type Props = { readonly topLevel?: boolean } & NavItemResponse;
-export const PluginMenuItem = ({ icon, name, topLevel = false, url_route: urlRoute, ...rest }: Props) => {
+export const PluginMenuItem = ({
+ icon,
+ // eslint-disable-next-line @typescript-eslint/naming-convention
+ icon_dark_mode,
+ name,
+ topLevel = false,
+ url_route: urlRoute,
+ ...rest
+}: Props) => {
// Determine if this is an external view or react app based on the presence of href
+ const { colorMode } = useColorMode();
const isExternalView = "href" in rest;
const href = isExternalView ? (rest as ExternalViewResponse).href : undefined;
const pluginIcon =
- typeof icon === "string" ? (
+ colorMode === "dark" && typeof icon_dark_mode === "string" ? (
+ <Image height="1.25rem" mr={topLevel ? 0 : 2} src={icon_dark_mode} width="1.25rem" />
Review Comment:
We could make two icon use the same component which makes it easier to maintain in the future!
```js
<Image height="1.25rem" mr={topLevel ? 0 : 2} src={colorMode === "dark" && typeof icon_dark_mode === "string" ? icon_dark_mode : icon} width="1.25rem" />
```
or
```js
const isDarkMode = colorMode === "dark" && typeof icon_dark_mode === "string"
<Image height="1.25rem" mr={topLevel ? 0 : 2} src={isDarkMode ? icon_dark_mode : icon} width="1.25rem" />
```
--
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] Fix handling of dark icons for plugins in nav [airflow]
Posted by "guan404ming (via GitHub)" <gi...@apache.org>.
guan404ming commented on code in PR #54305:
URL: https://github.com/apache/airflow/pull/54305#discussion_r2265369783
##########
airflow-core/src/airflow/ui/src/layouts/Nav/PluginMenuItem.tsx:
##########
@@ -23,20 +23,30 @@ import { RiArchiveStackLine } from "react-icons/ri";
import { Link as RouterLink } from "react-router-dom";
import type { ExternalViewResponse } from "openapi/requests/types.gen";
+import { useColorMode } from "src/context/colorMode";
import type { NavItemResponse } from "src/utils/types";
import { NavButton } from "./NavButton";
type Props = { readonly topLevel?: boolean } & NavItemResponse;
-export const PluginMenuItem = ({ icon, name, topLevel = false, url_route: urlRoute, ...rest }: Props) => {
+export const PluginMenuItem = ({
+ icon,
+ icon_dark_mode: iconDarkMode,
+ name,
+ topLevel = false,
+ url_route: urlRoute,
+ ...rest
+}: Props) => {
// Determine if this is an external view or react app based on the presence of href
+ const { colorMode } = useColorMode();
const isExternalView = "href" in rest;
const href = isExternalView ? (rest as ExternalViewResponse).href : undefined;
+ const someIcon = colorMode === "dark" && typeof iconDarkMode === "string" ? iconDarkMode : icon;
Review Comment:
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org