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.

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

Add Trigger DAG UI with advance options #43367

Open
wants to merge 45 commits into
base: main
Choose a base branch
from

Conversation

shubhamraj-git
Copy link
Contributor

@shubhamraj-git shubhamraj-git commented Oct 24, 2024

Screenshot 2024-11-13 at 1 06 13 AM Screenshot 2024-11-13 at 1 06 02 AM Screenshot 2024-11-13 at 1 05 31 AM Screenshot 2024-11-13 at 1 05 23 AM Screenshot 2024-11-13 at 1 05 14 AM Screenshot 2024-11-13 at 1 04 55 AM

^ 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 newsfragments.

@boring-cyborg boring-cyborg bot added the area:UI Related to UI/UX. For Frontend Developers. label Oct 24, 2024
@shubhamraj-git
Copy link
Contributor Author

@jscheffl Tried to cover many points. Still some points left.

  1. Reset should be shown only when "show advance option" is true.
  2. JSON linters and validation should be part of codemirror.
  3. Some style fix for config box.

Copy link
Contributor

@jscheffl jscheffl left a comment

Choose a reason for hiding this comment

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

Some early bird comments - mostly beautifying and details. Don't take all of them too serious, rather as hints.

airflow/ui/src/components/DataTable/TriggerDAG.ts Outdated Show resolved Hide resolved
airflow/ui/src/components/DataTable/TriggerDAG.ts Outdated Show resolved Hide resolved
airflow/ui/src/components/DataTable/TriggerDAGForm.tsx Outdated Show resolved Hide resolved
airflow/ui/src/components/DataTable/TriggerDAGForm.tsx Outdated Show resolved Hide resolved
airflow/ui/src/components/DataTable/TriggerDAGForm.tsx Outdated Show resolved Hide resolved
airflow/ui/src/components/DataTable/TriggerDAGForm.tsx Outdated Show resolved Hide resolved
airflow/ui/src/components/DataTable/TriggerDAGForm.tsx Outdated Show resolved Hide resolved
airflow/ui/src/components/DataTable/TriggerDAGModal.tsx Outdated Show resolved Hide resolved
airflow/ui/src/components/DataTable/TriggerDAGModal.tsx Outdated Show resolved Hide resolved
airflow/ui/src/pages/DagsList/DagsList.tsx Outdated Show resolved Hide resolved
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.

Let's move this from inside components/DataTable to components/TriggerDag

@bbovenzi
Copy link
Contributor

bbovenzi commented Nov 4, 2024

Oh you already rebased? How was moving your changes to chakra v3?

@shubhamraj-git
Copy link
Contributor Author

@bbovenzi
I rebased and encountered lot of issues. I am still working on getting the existing code to migrate to chakra ui 3.0, didn't pushed yet.
Some of the areas impacted are forms, modal.
Will refer the migration PR for the changes. Post that I will work on the reviews.

@bbovenzi
Copy link
Contributor

bbovenzi commented Nov 4, 2024

Ok. Let me know if you want me to pull the branch down and help with the rebase.

@shubhamraj-git
Copy link
Contributor Author

@bbovenzi @jscheffl Rebased successfully.
Last few commits just have rebase related changes. Will address the reviews in coming commits.

@shubhamraj-git shubhamraj-git marked this pull request as ready for review November 10, 2024 10:17
@shubhamraj-git shubhamraj-git changed the title DRAFT: Add trigger DAG with advance options Add trigger DAG with advance options Nov 10, 2024
Copy link
Contributor

@jscheffl jscheffl left a comment

Choose a reason for hiding this comment

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

Just one small bug and a small nit... otherwise looks very good and from my point could be merged!
Thanks!

@shubhamraj-git shubhamraj-git changed the title Add trigger DAG with advance options Add Trigger DAG UI with advance options Nov 12, 2024
</Heading>
{isPaused ? (
<Alert status="warning" title="Paused DAG">
You can not trigger a paused DAG, Unpause DAG to trigger
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
You can not trigger a paused DAG, Unpause DAG to trigger
Dag is paused. Triggering will create a Dag run but it won't start until the Dag is unpaused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, should we remove the disable trigger button on paused DAG? Since the wording let user trigger it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since, we have a title "Paused DAG"
Let's Include just "Triggering will create a Dag run but it won't start until the Dag is unpaused." in the subject ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

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