-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Add result count to dag run and task instance tabs #57680
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?
Add result count to dag run and task instance tabs #57680
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
guan404ming
left a comment
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.
Overall looks good. Small nit, I think we just need to output one not found message in UI is enough.
|
@cmbilly8 , Appreciate that you reused the existing state instead of adding extra API calls. One small thought for next time: adding a quick test or storybook screenshot for RTL/i18n coverage whenever we touch display components, just to keep those cases visible early. |
Thanks for your suggestion. IMO, to provide RTL screenshot is great, but I think i18n coverage is not required here since this PR is not related to translation change. |
3572ecc to
2122c12
Compare
|
Actually, one thought. We already pass the total_entries to . Perhaps we should make this part of the shared DataTable component so that it displays on everything, not just Dag Runs and Task Instances? |
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.
Actually, one thought. We already pass the total_entries to . Perhaps we should make this part of the shared DataTable component so that it displays on everything, not just Dag Runs and Task Instances?
I thought exactly the same. It will avoid code duplication and make things easier if the TableComponent just had a 'header' showing the total number. (And we also already pass down the modelName too.
And probably find a way to also incorporate the same from the Dags page, which is pretty similar.
<Heading py={3} size="md">
{`${data?.total_entries ?? 0} ${translate("dag", { count: data?.total_entries ?? 0 })}`}
</Heading>
2122c12 to
514b509
Compare
|
I refactored the DataTable to include the row count rendering logic, which involved moving the model name translation logic to within the DataTable so I could reuse the raw model name. I also added the row count heading to the pages that it seemed appropriate on:
And updated the following pages that already show row counts to use the heading from the DataTable instead:
The only one that was not straightforward was the DagsList because it places the ToggleTableDisplay component between the row count heading and table (The screenshot shows how it shifted to above the row count heading). Since this component is only used on the DagsList page, does it make sense to support that layout through moving that toggle to an optional prop on the DataTable or should it be slotted in by the caller? Open to other approaches as well. |
Great work. Let's make it an optional prop so that we're not creating excess vertical space on the dags list page. Maybe in the future other table views will have card list variants. |
|
@cmbilly8 Care to rebase? This would still be nice to merge. |
da1f3c7 to
19c5fc3
Compare
|
I noticed I had also inadvertently added additional whitespace to the assets page and altered some alignment in the dags list while testing the prop for the ToggleTableDisplay. Since it was just those two pages, I kept their own row count rendering logic so that the pages could manager their own layouts, while the other pages still use the prop on the Datatable. (Also rebased on main) |





Description
This PR adds the total element count to the Dag Runs and Task Instances grids, similar to the existing behavior in the Dags tab.
No new data fetching or logic was added; the count uses an already available attribute in the component's state.
Closes #57639
UI Preview
Dag Runs
Task Instances
Testing
Manually verified across multiple states and views.
DAG Runs:
Task Instances:
DAG Page (Dag Runs tab):

DAG Run Page (Task Instances tab):

RTL / i18n Validation:
^ 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.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.