Skip to content

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jscheffl
Copy link
Contributor

@jscheffl jscheffl commented Jun 17, 2025

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

@boring-cyborg boring-cyborg bot added the area:UI Related to UI/UX. For Frontend Developers. label Jun 17, 2025
@bbovenzi bbovenzi changed the title Impreve Dag run and Task Instance notes Improve Dag run and Task Instance notes Jun 18, 2025
@bbovenzi
Copy link
Contributor

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

Copy link
Member

@pierrejeambrun pierrejeambrun left a 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 = ({
Copy link
Member

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.

@jscheffl
Copy link
Contributor Author

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.

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.

3 participants