Skip to content
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.

Sign up for GitHub

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

Jump to bottom

confirmation dialog box for DAG run actions #35393

Merged
merged 12 commits into from
Feb 12, 2024
Merged

confirmation dialog box for DAG run actions #35393

merged 12 commits into from
Feb 12, 2024

Conversation

theaadya
Copy link
Contributor

@theaadya theaadya commented Nov 3, 2023

closes: #34685

created a dialog box which asks for user confirmation before clearing a DAG run or marking it
please review

@boring-cyborg boring-cyborg bot added area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues labels Nov 3, 2023
Taragolis
Taragolis approved these changes Nov 5, 2023
View reviewed changes
Copy link
Contributor

@Taragolis Taragolis left a comment

Choose a reason for hiding this comment

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

I'm not familiar with TypeScript, so I can't review code itself.

However changes works fine into the UI

image

image

image

potiuk
potiuk approved these changes Nov 5, 2023
View reviewed changes
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Yeah. looks good - but I'd prefer a "UI savy" maintainer to confirm :)

@amoghrajesh
Copy link
Contributor

@bbovenzi @pierrejeambrun Can you take a look at this PR?
@potiuk i think this is a feature, so should we be adding semver for the UI as well? Or its ok to ship this in a minor release too?

@bbovenzi bbovenzi changed the title confirmation dialog box for DAG keyboard shortcuts confirmation dialog box for DAG run actions Nov 15, 2023
@eladkal eladkal added this to the Airflow 2.8.0 milestone Nov 22, 2023
amoghrajesh
amoghrajesh approved these changes Nov 23, 2023
View reviewed changes
Copy link
Contributor

@amoghrajesh amoghrajesh left a comment

Choose a reason for hiding this comment

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

Nice. I am ok with the PR. LGTM +1
@bbovenzi / @pierrejeambrun?

pierrejeambrun
pierrejeambrun reviewed Nov 26, 2023
View reviewed changes
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Sorry for the late review.

I feel like having 3 separate values for persistence of user preference for the 3 different types of modal is a bit overkill.

Maybe this could just be a common single doNotShowAgain.... Having the user tick that box 3 times to get rid of the modals is maybe a little redundant. (but I am not against it)

Just a couple suggestions, but looking good overall :)

airflow/www/static/js/dag/details/dagRun/MarkRunAs.tsx Show resolved Hide resolved
airflow/www/static/js/dag/details/dagRun/MarkRunAs.tsx Outdated Show resolved Hide resolved
@eladkal eladkal requested a review from bbovenzi December 5, 2023 19:20
@theaadya
Copy link
Contributor Author

theaadya commented Dec 7, 2023

Done with the requested changes!

@amoghrajesh
Copy link
Contributor

@bbovenzi @pierrejeambrun can you take another look at this when you have some time?

@amoghrajesh
Copy link
Contributor

@bbovenzi @potiuk are we good for a merge here?

@potiuk
Copy link
Member

potiuk commented Jan 30, 2024

Something failed last time - looks like intermittent EOF on docker build. I rebased to see if it is OK

@bbovenzi
Copy link
Contributor

Something failed last time - looks like intermittent EOF on docker build. I rebased to see if it is OK

I just rebased again. Will merge if everything is green.

@bbovenzi bbovenzi merged commit 002d15a into apache:main Feb 12, 2024
53 checks passed
@ephraimbuddy ephraimbuddy added the type:improvement Changelog: Improvements label Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eladkal eladkal eladkal approved these changes

@amoghrajesh amoghrajesh amoghrajesh approved these changes

@bbovenzi bbovenzi bbovenzi approved these changes

@Taragolis Taragolis Taragolis approved these changes

@potiuk potiuk potiuk approved these changes

@ryanahamilton ryanahamilton Awaiting requested review from ryanahamilton ryanahamilton is a code owner

@ashb ashb Awaiting requested review from ashb ashb is a code owner

@pierrejeambrun pierrejeambrun Awaiting requested review from pierrejeambrun pierrejeambrun is a code owner

Assignees
No one assigned
Labels
area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues type:improvement Changelog: Improvements
Projects
None yet
Milestone
Airflow 2.9.0
Development

Successfully merging this pull request may close these issues.

Keyboard shortcuts opens a modal window for DAG operations
8 participants
@theaadya @amoghrajesh @potiuk @bbovenzi @Taragolis @pierrejeambrun @eladkal @ephraimbuddy