Re: [PR] UI: Implement navigation on bar click [airflow]
bbovenzi merged PR #50416: URL: https://github.com/apache/airflow/pull/50416 -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] UI: Implement navigation on bar click [airflow]
RoyLee1224 commented on PR #50416: URL: https://github.com/apache/airflow/pull/50416#issuecomment-2869970585 Thanks for the detailed feedback! -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] UI: Implement navigation on bar click [airflow]
RoyLee1224 commented on code in PR #50416:
URL: https://github.com/apache/airflow/pull/50416#discussion_r2083283292
##
airflow-core/src/airflow/ui/src/components/DurationChart.tsx:
##
@@ -141,6 +144,29 @@ export const DurationChart = ({
}}
datasetIdKey="id"
options={{
+ onClick: (_event, elements) => {
+const [element] = elements;
+
+if (!element) {
+ return;
+}
+if (kind === "Dag Run") {
+ const entry = entries[element.index];
+ const run = entry as DAGRunResponse;
+
+ navigate(`/dags/${run.dag_id}/runs/${run.dag_run_id}`);
Review Comment:
That makes sense, thanks for the tip!
##
airflow-core/src/airflow/ui/src/components/DurationChart.tsx:
##
@@ -141,6 +144,29 @@ export const DurationChart = ({
}}
datasetIdKey="id"
options={{
+ onClick: (_event, elements) => {
+const [element] = elements;
+
+if (!element) {
+ return;
+}
+if (kind === "Dag Run") {
Review Comment:
Agree, done
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] UI: Implement navigation on bar click [airflow]
guan404ming commented on code in PR #50416:
URL: https://github.com/apache/airflow/pull/50416#discussion_r2083270673
##
airflow-core/src/airflow/ui/src/components/DurationChart.tsx:
##
@@ -141,6 +144,29 @@ export const DurationChart = ({
}}
datasetIdKey="id"
options={{
+ onClick: (_event, elements) => {
+const [element] = elements;
+
+if (!element) {
+ return;
+}
+if (kind === "Dag Run") {
Review Comment:
nit: maybe we could use `switch` like line 109 to preserve the coding style
consistency and extensibility
##
airflow-core/src/airflow/ui/src/components/DurationChart.tsx:
##
@@ -141,6 +144,29 @@ export const DurationChart = ({
}}
datasetIdKey="id"
options={{
+ onClick: (_event, elements) => {
+const [element] = elements;
+
+if (!element) {
+ return;
+}
+if (kind === "Dag Run") {
+ const entry = entries[element.index];
+ const run = entry as DAGRunResponse;
+
+ navigate(`/dags/${run.dag_id}/runs/${run.dag_run_id}`);
Review Comment:
nit: using like `baseUrl = /dags/${run.dag_id}/runs/` to store common path
would be easier to maintain if we have more cases added in the future.
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] UI: Implement navigation on bar click [airflow]
RoyLee1224 commented on PR #50416: URL: https://github.com/apache/airflow/pull/50416#issuecomment-2869083019 Implemented click-to-navigate for task instances as well. https://github.com/user-attachments/assets/cafacc04-c322-4232-ad58-7744dbc6710a -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] UI: Implement navigation on bar click [airflow]
guan404ming commented on code in PR #50416:
URL: https://github.com/apache/airflow/pull/50416#discussion_r2083206425
##
airflow-core/src/airflow/ui/src/components/DurationChart.tsx:
##
@@ -141,6 +145,24 @@ export const DurationChart = ({
}}
datasetIdKey="id"
options={{
+ onClick: (_event, elements) => {
+const [element] = elements;
+
+if (element && typeof element.index === "number") {
+ const entry = entries[element.index];
+
+ if (kind === "Dag Run") {
Review Comment:
No worries, feel free to tag me if there is anything I could help.
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] UI: Implement navigation on bar click [airflow]
RoyLee1224 commented on code in PR #50416:
URL: https://github.com/apache/airflow/pull/50416#discussion_r2083204514
##
airflow-core/src/airflow/ui/src/components/DurationChart.tsx:
##
@@ -141,6 +145,24 @@ export const DurationChart = ({
}}
datasetIdKey="id"
options={{
+ onClick: (_event, elements) => {
+const [element] = elements;
+
+if (element && typeof element.index === "number") {
+ const entry = entries[element.index];
+
+ if (kind === "Dag Run") {
Review Comment:
After further testing, I realize I was incorrect.
We do need to handle the Task Instance case in the onClick handler. Let me
fix it.
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] UI: Implement navigation on bar click [airflow]
RoyLee1224 commented on code in PR #50416:
URL: https://github.com/apache/airflow/pull/50416#discussion_r2083137702
##
airflow-core/src/airflow/ui/src/components/DurationChart.tsx:
##
@@ -141,6 +145,24 @@ export const DurationChart = ({
}}
datasetIdKey="id"
options={{
+ onClick: (_event, elements) => {
+const [element] = elements;
+
+if (element && typeof element.index === "number") {
+ const entry = entries[element.index];
+
+ if (kind === "Dag Run") {
Review Comment:
I think adding Task Instance case would create duplicate navigation logic.
The `TaskRecentRuns` component (used in Tasks page) already implements
navigation using React Router's Link component
For context, the `DurationChart` component is used in two different contexts:
- In the `Overview` page for DAG Runs: we use the onClick handler in
react-chartjs-2 Bar to navigate
- In the `Tasks` page for Task Instances: the navigation is already handled
by the Link component in the TaskRecentRuns component
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] UI: Implement navigation on bar click [airflow]
RoyLee1224 commented on code in PR #50416:
URL: https://github.com/apache/airflow/pull/50416#discussion_r2083204514
##
airflow-core/src/airflow/ui/src/components/DurationChart.tsx:
##
@@ -141,6 +145,24 @@ export const DurationChart = ({
}}
datasetIdKey="id"
options={{
+ onClick: (_event, elements) => {
+const [element] = elements;
+
+if (element && typeof element.index === "number") {
+ const entry = entries[element.index];
+
+ if (kind === "Dag Run") {
Review Comment:
I apologize for my previous response. After further testing, I realize I was
incorrect.
We do need to handle the Task Instance case in the onClick handler. Let me
fix it.
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] UI: Implement navigation on bar click [airflow]
RoyLee1224 commented on code in PR #50416:
URL: https://github.com/apache/airflow/pull/50416#discussion_r2083137702
##
airflow-core/src/airflow/ui/src/components/DurationChart.tsx:
##
@@ -141,6 +145,24 @@ export const DurationChart = ({
}}
datasetIdKey="id"
options={{
+ onClick: (_event, elements) => {
+const [element] = elements;
+
+if (element && typeof element.index === "number") {
+ const entry = entries[element.index];
+
+ if (kind === "Dag Run") {
Review Comment:
I think adding Task Instance case would create duplicate navigation logic.
The `TaskRecentRuns` component (used in Tasks page) already implements
navigation using React Router's Link component
For context, the `DurationChart` component is used in two different contexts:
- In the `Overview` page for DAG Runs: we use the onClick handler in
react-chartjs-2 Bar to navigate
- In the `Tasks` page for Task Instances: the navigation is already handled
by the Link component in the TaskRecentRuns component
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] UI: Implement navigation on bar click [airflow]
RoyLee1224 commented on code in PR #50416:
URL: https://github.com/apache/airflow/pull/50416#discussion_r2083135759
##
airflow-core/src/airflow/ui/src/components/DurationChart.tsx:
##
@@ -141,6 +145,24 @@ export const DurationChart = ({
}}
datasetIdKey="id"
options={{
+ onClick: (_event, elements) => {
+const [element] = elements;
+
+if (element && typeof element.index === "number") {
Review Comment:
You're right, I thought we needed the type assertion for edge cases like
clicking annotations, but I now understand that clicking an annotation doesn't
provide an element.
I'll remove it, 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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] UI: Implement navigation on bar click [airflow]
guan404ming commented on code in PR #50416:
URL: https://github.com/apache/airflow/pull/50416#discussion_r2083003121
##
airflow-core/src/airflow/ui/src/components/DurationChart.tsx:
##
@@ -141,6 +145,24 @@ export const DurationChart = ({
}}
datasetIdKey="id"
options={{
+ onClick: (_event, elements) => {
+const [element] = elements;
+
+if (element && typeof element.index === "number") {
+ const entry = entries[element.index];
+
+ if (kind === "Dag Run") {
Review Comment:
Should we also need to handle case task here?
##
airflow-core/src/airflow/ui/src/components/DurationChart.tsx:
##
@@ -141,6 +145,24 @@ export const DurationChart = ({
}}
datasetIdKey="id"
options={{
+ onClick: (_event, elements) => {
+const [element] = elements;
+
+if (element && typeof element.index === "number") {
Review Comment:
Not sure if the type assertion is necessary. What other types could the
index type have?
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
