Skip to content

Conversation

jsjasonseba
Copy link
Contributor

@jsjasonseba jsjasonseba commented Apr 15, 2025

closes: #49158

Implemented selectable task nodes by wrapping flex with router link.
Also adds isLinkDisabled to prevent nested link warning


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

@boring-cyborg boring-cyborg bot added the area:UI Related to UI/UX. For Frontend Developers. label Apr 15, 2025
Copy link
Contributor

@bbovenzi bbovenzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally we shouldn't have too much stuff inside of an <a> tag. Which, we should change our use of TaskLink here so we don't have two links.

Screenshot 2025-04-15 at 11 25 17 AM

Also, let's update the other node components for a consistent UX.

@jsjasonseba
Copy link
Contributor Author

Generally we shouldn't have too much stuff inside of an <a> tag. Which, we should change our use of TaskLink here so we don't have two links.

Screenshot 2025-04-15 at 11 25 17 AM Also, let's update the other node components for a consistent UX.

Hi @bbovenzi , I added a flag in TaskLink to disable the router link when defined by TaskNode. I think I missed a case, where do you find this error?

@jsjasonseba
Copy link
Contributor Author

jsjasonseba commented Apr 15, 2025

But I guess, I should just use Button instead of another Router Link. Will change it shortly

@bbovenzi
Copy link
Contributor

Hi @bbovenzi , I added a flag in TaskLink to disable the router link when defined by TaskNode. I think I missed a case, where do you find this error?

In the browser console tab for any dag graph. Its not just the disabled part but having inside of an because of TaskLink

@jsjasonseba jsjasonseba force-pushed the selectable-task-node branch from b1c77cd to fa7fdaa Compare April 15, 2025 16:48
@jsjasonseba
Copy link
Contributor Author

@bbovenzi using chakra LinkOverlay works! Such a simple and clean solution.

I applied this to DagNode, TaskNode, and AssetNode.
For task groups, currently it still requires user to click the + .. task button, but I think that interaction still makes sense in the context of task groups.

@jsjasonseba jsjasonseba requested a review from bbovenzi April 15, 2025 16:54
@bbovenzi
Copy link
Contributor

Making progress! A few more notes:

  • We need a event.stopProgagation() for DagNode so we can toggle them
  • We should drop the ` component. We no longer need it to highlight or be blue
  • Same for TaskLink we should just show the task name
Screenshot 2025-04-16 at 12 54 48 PM

@jsjasonseba jsjasonseba force-pushed the selectable-task-node branch from 4ffe6d6 to 5bd1437 Compare April 18, 2025 17:43
@jsjasonseba
Copy link
Contributor Author

Hi @bbovenzi , I wasn't able to make it work with event.stopPropagation(). It seems that LinkOverlay covers the entire box with the <a> tag. However, I managed to make it work by applying z-index to the toggle. Feel free to review these changes.

@jsjasonseba
Copy link
Contributor Author

Hi @bbovenzi , the changes are done. Feel free to review at your convenience.

Copy link
Contributor

@bbovenzi bbovenzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

Just one last issue.

We need to forward the ref from the LinkOverlay.

Replace line 27 of TaskLink.tsx with export const TaskLink = forwardRef<HTMLAnchorElement, Props>(({ id, isGroup, isMapped, ...rest }, ref) => {

@jsjasonseba
Copy link
Contributor Author

Hi @bbovenzi , I’ve made the requested changes. Just learned about forwardRef too! Thanks for feedback, I'm still getting the hang of React, especially coming from Vue.

@bbovenzi bbovenzi added this to the Airflow 3.0.1 milestone Apr 23, 2025
Copy link
Contributor

@bbovenzi bbovenzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Thanks for your patience on this one. I think a lot of people will appreciate this UX change.

And no worries on forwardRef, thats a very specific react concept

@bbovenzi bbovenzi merged commit 20bdf5f into apache:main Apr 23, 2025
42 checks passed
kaxil pushed a commit that referenced this pull request Apr 23, 2025
#49299)

* feature(ui): Make entire task node clickable to select the task

* Add z-index to TogglePause for clickability

* Remove link component

* Remove unnecessary onClick handler from LinkOverlay

* Forward ref from LinkOverlay

(cherry picked from commit 20bdf5f)
prabhusneha pushed a commit to astronomer/airflow that referenced this pull request Apr 25, 2025
apache#49299)

* feature(ui): Make entire task node clickable to select the task

* Add z-index to TogglePause for clickability

* Remove link component

* Remove unnecessary onClick handler from LinkOverlay

* Forward ref from LinkOverlay
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:UI Related to UI/UX. For Frontend Developers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make entire task box clickable to select the task in Airflow 3 graph view
2 participants