ryanahamilton commented on a change in pull request #11652:
URL: https://github.com/apache/airflow/pull/11652#discussion_r518387513
##########
File path: airflow/www/templates/airflow/dags.html
##########
@@ -20,6 +20,51 @@
{% extends base_template %}
{% from 'appbuilder/loading_dots.html' import loading_dots %}
+{%- macro verbose_ordering_name(ordering) -%}
+ {{ 'descending' if ordering == 'desc' else ('ascending' if ordering ==
'asc' else ordering) }}
+{%- endmacro -%}
+
+{%- macro sorted_th_link(name, attr_name) -%}
+ {% set request_ordering = (request.args.get('orderBy', 'unsorted')) %}
+ {% set curr_ordering = ('unsorted' if request.args.get('sortBy') !=
attr_name else request_ordering ) %}
+ {% set new_ordering = ('asc' if request.args.get('sortBy') != attr_name or
curr_ordering != 'asc' else 'desc' ) %}
+ <a href="{{ url_for('Airflow.index',
+ status=request.args.get('status', 'all'),
+ search=request.args.get('search', None),
+ tags=request.args.get('tags', None),
+ sortBy=attr_name,
+ orderBy=new_ordering
+ ) }}">
+ {{ name }}
+ <span id="sorting-tip-{{ name }}" class="tooltip" role="tooltip" >
+ {% if curr_ordering == 'unsorted' %}
+ Column not currently sorted.
+ {% else %}
+ Column sorted by {{ attr_name }}, {{
verbose_ordering_name(curr_ordering) }}.
+ {% endif %}
+ Press to sort by {{ verbose_ordering_name(new_ordering) }} {{ attr_name
}}.
+ </span>
+ </a>
+ <a href="{{ url_for('Airflow.index',
+ status=request.args.get('status', 'all'),
+ search=request.args.get('search', None),
+ tags=request.args.get('tags', None),
+ sortBy=attr_name,
+ orderBy=new_ordering
+ ) }}">
+ <!-- We need a separate <a> here because otherwise some screen readers
STILL pick it up for some reason >_< -->
Review comment:
Curious about this… the extra anchor seems like a lot of extra markup
overhead. I just tested this my self using ChromeVox Classic Extension. The
`aria-hidden="true"` was working as expected.
##########
File path: airflow/www/templates/airflow/dags.html
##########
@@ -20,6 +20,51 @@
{% extends base_template %}
{% from 'appbuilder/loading_dots.html' import loading_dots %}
+{%- macro verbose_ordering_name(ordering) -%}
+ {{ 'descending' if ordering == 'desc' else ('ascending' if ordering ==
'asc' else ordering) }}
+{%- endmacro -%}
+
+{%- macro sorted_th_link(name, attr_name) -%}
+ {% set request_ordering = (request.args.get('orderBy', 'unsorted')) %}
+ {% set curr_ordering = ('unsorted' if request.args.get('sortBy') !=
attr_name else request_ordering ) %}
+ {% set new_ordering = ('asc' if request.args.get('sortBy') != attr_name or
curr_ordering != 'asc' else 'desc' ) %}
+ <a href="{{ url_for('Airflow.index',
+ status=request.args.get('status', 'all'),
+ search=request.args.get('search', None),
+ tags=request.args.get('tags', None),
+ sortBy=attr_name,
+ orderBy=new_ordering
+ ) }}">
+ {{ name }}
+ <span id="sorting-tip-{{ name }}" class="tooltip" role="tooltip" >
Review comment:
You can simply this and circumvent adding this tooltip span by adding a
`class="js-tooltip"` and a `title="{{ tooltip }}"` attributes to the parent
anchor.
IMO, the current tooltip text is a bit too verbose. I would suggest dropping
the "Column not currently sorted." and "Column sorted by…" all together and
just provide description of what clicking on will do: "Sort by ascending owner"
etc.
```jinja
<a
…
class="js-tooltip"
title="Sort by {{ verbose_ordering_name(new_ordering) }} {{ attr_name }}"
>
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
