-
Notifications
You must be signed in to change notification settings - Fork 16.4k
UX improvements of DAG tag filtering #11661
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
|
I think the reason for the |
|
@ashb yeah, I understand that desire, but I believe it to be anti-pattern for filtering to not have continuity with the URL params. When a user utilizes the back button, their previously applied tag filters will still persist. |
|
From a user POV the existing behaviour is very useful though -- espeically if you have a large Airflow deployment, and you only want to see "your team's" DAGs -- having to re-apply the filter each time you visit the page would be tedious. |
|
@ash I see that even though "tags" are typically an unspecified feature use, this feature was originally intended for this "team" use-case, which makes more sense to persist the filter. I'll see about refactoring this to keep the cookie in place. |
|
I can see the benefits of @ryanahamilton 's solution, but also understand the needs of some users presented by @ashb who wanted selected tags to be remembered at all times. Maybe we can combine these two ideas and let the user keep the currently selected tags as default? I see it this way, that the user can browse DAGs normally according to @ryanahamilton 's suggestion, but if user wants to configure, be able to quickly return to the selected configuration, all users has to do is press the "Save as default" button. It doesn't even have to be on this page, but it could be a configuration option in your user profile settings ("Default selected tags" or something similar). I think this will better address the needs of both user groups. |
|
@mik-laj I like that idea. It might make sense to save the status (all/active/paused) in the "default" preference as well since we're also cookie-ing that value? That feels like it should be addressed as a feature in a subsequent PR—I think I can offer a refactor to this PR that maintains the current user expectations but still delivers on the UX improvements. |
|
@ryanahamilton I have no preference. Each solution works for me. Let me know when your change is ready for review. |
|
I reverted the removal of the cookie storage, but I've updated it so that if cookie values exist, the I also made an update to strip the |
zacharya19
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.
LGTM, a few nits, but I really like the improvements!
f055179 to
cdfbed5
Compare
ashb
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.
Nice approach
Removes the storage of the tags in theUpdated the server-side logic, so that if cookie values exist, thetags_filtercookie. For a filtering pattern such as this, the URL params should always reflect the current result set. Currently, you can have your result set filtered by tags and have notags=params in your URL. This also required using a&reset_tags=Resetparam to clear the values—this will no longer be needed with the URL explicitly reflecting the values.tags=param is added to the URL so it matches the filtered result set. I also made an update to strip thereset_tags=resetfrom the URL after the cookie value has been cleared.CC: @zacharya19 thought you might be interested in these modifications to your feature originally added in #6489.