-
Notifications
You must be signed in to change notification settings - Fork 15.2k
Improve Dag run and Task Instance notes #51859
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This is better. I still don't like the Tabs moving up and down. I might pull your branch down and play around with the UX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the follow up. I think this can be a good alternative compare to what we currently have on main.
I see that you also figured out that plenty of images would just make the details tab unusable without this PR. (I was about to create an issue about it).
Not approving to let Brent time to investigate an alternate solution.
import EditableMarkdownArea from "./EditableMarkdownArea"; | ||
import { Accordion } from "./ui/Accordion"; | ||
|
||
const EditableMarkdownNotes = ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the name of the component is super descriptive. (vs EditableMarkdownArea
)
Because it leaves us with Area
vs Notes
, impossible to guess who is collapsible, who's not, what's the difference.
Yeah, thanks for the feedback. Feel free to pull it down and experiment with it. I am not sure where but certain there mgiht be ready-made components in the React ecosystem which would allow an expansion on demand besides the accordion. So I think both commeonts from you are valid and just wanted to signal that I am okay to improve... just my React skills ight be limited to the level like in this PR :-D Yeah and of course we can also leave the maxHeight to a static limit as an increment... I assume the example with the images is theoretically possible but a very rare/extreme case. Usually I'd expect a few words or notes and as the field is limited to 1000 chars... ~300px might be a realistic size and scrollbars might be also sufficient not to make it overly complex. So yeah, constructive ideas welcome, no pressure but would be good to make it "pretty" before 3.1. |
Follow-up of #51764
As @pierrejeambrun and @bbovenzi had some feedback, this adds an accordion and limits the maximum space used, add an option to hide notes.
Better like this?
dag_run_notes2-2025-06-17_22.47.56.mp4