-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Implement UI/UX improvements for the counter in the Dags list #56067
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
Conversation
6c8fd5b to
92fbf14
Compare
3fa6aa3 to
059dd9f
Compare
jscheffl
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.
Looks good for me - but another pair of GUI Expertise might be good.
059dd9f to
8ef1b40
Compare
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.
Caching the current number of Dags, so in the next time the page reloads - it will show it instead of a plain 0, before rendering completes.
That I don't think we should do that. It adds complexity and possible desync value for not a lot of gain since the total_entries is part of the response. Also if I hard refresh I expect to see a '0' testifying that I'm not using any cached data. That would also remove the useEffect I hope which is something we try to avoid.
Animating the counter for smooth transition between 0 or the cached value until the current state :)
For the animation part I've utilized countUp.js package, which provides a lightweight implementation for such animations. It seems to be maintained, widely-used (131K), and with a suitable license (MIT).
That's cool. If we do that here we probably want that everywhere.
Formatting the number using a thousands seperator according to the current locale - initially I thought of hardcoding them, but I've managed to use native i18n functions instead (for better compatibiltiy, we might want in the future to rename the locale codes according to IETF BCP 47 - e.g., Polish should be renamed as pl-PL).
Same super cool and we probably want that everywhere we display numbers.
Maybe we should split this PR per 1,2,3 because it would need to be application wide and many places will need to be upgraded to stay consistant.
Thanks for the detailed review!
|
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
pierrejeambrun
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.
Just to follow up on this, because I think we could make a quick win.
To bring value incrementally, I would focus this PR on adding a simple "count" displaying the total number of 'items' as part of the 'DataTable' component. (So every tables now shows the total number of items at the top).
I would get rid of any caching and animations, so we can quickly get this merge. We can follow up with such improvement afterward.
Hey, wasn't focused on the UI area lately - I'm into the quick win as well, I'll try to get back to it ASAP |
|
@shahar1 For the 'quick win part' I just realized that there is #57680 opened that will cover this and is close to being ready, so you don't even need to do it. I'll close this PR, feel free to re-open or open a new one if you want to tackle the follow up items. (animations, caching, that you mentioned above) |
This PR implements some small UI/UX improvements for the Dags counter, in the Dags list:
0, before rendering completes.For the animation part I've utilized countUp.js package, which provides a lightweight implementation for such animations. It seems to be maintained, widely-used (131K), and with a suitable license (MIT).
i18nfunctions instead (for better compatibiltiy, we might want in the future to rename the locale codes according toIETF BCP 47- e.g., Polish should be renamed aspl-PL).In future work, we could extract the functionalities to an external function/hook and implement the changes in other counters as well.
Demo
Note: the animation looks a bit twitching - tried to fix it :)
Airflow.-.Google.Chrome.2025-09-24.19-13-47.mp4
^ 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.