grnet / djnro

DjNRO hits the decks of eduroam database management
http://djnro.grnet.gr/
Other
10 stars 21 forks source link

Connect: visually distinguish CAT-bound institutions #60

Closed vladimir-mencl-eresearch closed 5 years ago

vladimir-mencl-eresearch commented 5 years ago

Hi @zmousm ,

Thanks for the work on #57 - that has finally made CAT usable for us.

And we started using it. And we realized we need to help our users navigate through the Connect page - we thought we should increase the visual difference between CAT-enabled institutions and those that just link to the Participants page.

I've added a light border around the CAT-enabled ones, plus added an icon for them (ended up picking "wifi" from FA) to be the counterpart of the link arrow. Than just had to add data-toggle attribute for the non-CAT institutions as well to allow positive selection in the CSS.

Please let me know if this looks OK to you to merge (on visual and code aspects).

Thanks a lot in advance for getting back to me.

Cheers, Vlad

vladimir-mencl-eresearch commented 5 years ago

Hi @zmousm , FYI, you can see this in action at https://member.eduroam.net.nz/connect - Screenshot attached. Screen Shot 2019-05-07 at 11 10 22

zmousm commented 5 years ago

Hello @vladimir-mencl-eresearch. I'm glad you found eduroam connect useful :)

This is how your patch looks on eduroam.gr:

eduroam.gr/connect

I think your visual tweaks make the institution list look a bit too busy. Mostly all our member institutions do use CAT already, but this is apparently not yet the case for your federation, so it is understandable the visual distinction makes more sense there. In this respect I think such tweaks are not suitable for general consumption, and I'm open to discuss if you feel otherwise, but let's see how you can have them in your deployments, without them getting in the way.

We had built in a mechanism which allows overriding parts of the template for this page, through the use of block tags. It is documented here:

https://github.com/grnet/djnro/blob/master/docs/installation/install.md#connect-page-customization-for-different-eduroam-cat-instances

You can add a cat_institution_selector block to power this case:

--- a/djnro/templates/front/connect.html
+++ b/djnro/templates/front/connect.html
@@ -68,6 +68,7 @@
        {% for i in institutions %}
        {% with catidp=institutions_cat|dict_lookup:i.institution.pk %}
        <li>
+         {% block cat_institution_selector %}
          {% if catidp and catexists %}
          <a class="btn" data-toggle="modal" data-target="catModal"
             data-idu="{% url 'participants' %}"
@@ -78,6 +79,7 @@
          {% endif %}
          <span class="title" {% if i.oper_name %}data-on="{{i.oper_name}}"{% endif %}>{% tolocale i.institution LANGUAGE_CODE %}</span><span class="small-small hidden">&nbsp;<sp
          </a>
+         {% endblock %}
        </li>
        {% endwith %}
        {% endfor %}

We can merge this in DjNRO (please remember to also document this block).

You can then create djnro/templates/front/cat_connect/connect.html with such content:

{% extends "front/connect.html" %}
{% load staticfiles %}
{% load i18n %}
{% load tolocale %}

{% block cat_institution_selector %}
      {% if catidp and catexists %}
      <a class="btn" data-toggle="modal" data-target="catModal"
         data-idu="{% url 'participants' %}"
         data-iid="{{ i.institution.pk }}" data-catidp="{{catidp}}" tabindex="0">
      {% else %}
      <a class="btn" data-iid="{{ i.institution.pk }}"
         href="{% url 'participants' %}" tabindex="0">
      {% endif %}
      <span class="title" {% if i.oper_name %}data-on="{{i.oper_name}}"{% endif %}>{% tolocale i.institution LANGUAGE_CODE %}</span><span class="small-small hidden">&nbsp;<span class="distance"></span>&nbsp;Km</span>&nbsp;<i title="{% trans 'See info (instructions, contacts) for this institution' %}" data-toggle="tooltip" data-placement="bottom" class="fa fa-arrow-circle-right text-muted connect-link"></i><i title="Use CAT profile created for this institution" data-toggle="tooltip" data-placement="bottom" class="fa fa-wifi text-muted connect-cat"></i>
      </a>
    </li>
{% endblock %}

{% block extrahead %}
<link rel='stylesheet' href="{% static 'css/nprogress.css' %}">
<link rel='stylesheet' href="{% static 'css/connect.css' %}">
<style>
/* when an institution selector is CAT-bound, the following arrow
   (visual distinction) should be hidden */
ul.insts a[data-toggle="modal"] i.connect-link {
    display: none;
}
/* and instead a different icon shown */
ul.insts a[data-toggle="modal"] i.connect-cat {
    display: inline;
}
/* but this icon should be hidden otherwise */
ul.insts a:not([data-toggle="modal"]) i.connect-cat {
    display: none;
}

/* distinguish CAT-enabled institutions */
ul.insts a[data-toggle="modal"] {
    border: .1rem solid green;
}
</style>
{% endblock %}

And point to it in your settings:

CAT_CONNECT_TEMPLATE = {
    'production': 'front/cat_connect/connect.html',
}

Some notes:

vladimir-mencl-eresearch commented 5 years ago

Hi @zmousm , thanks a lot for the very quick and very helpful reply!

Agree that in your case, this would look crowded. I'll try to think of of something that would apply only to us ... and minimize the "get in the way part".

In this PR, I ended up doing rather complex gymnastics to introduce a second icon for the CAT-enabled institutions.

The reason for having the second icon was to give a clue to the users that these institutions are CAT enabled.

Institutions that are NOT CAT-enabled end up with the arrow that says "You can't use CAT, you have to follow a link". And to avoid relying only on the visual clue of the arrow, there is a tooltip shown over the arrow.

I wanted to introduce an equivalent of that for CAT-enabled institutions: an icon that says "you can use CAT here", and a place where to put the tooltip with the explanatory text.

Putting the visual details (and the particularly the green frame) aside, would you think of this as generally useful, or rather not?

(I'm leading this towards merging the "wifi" icon, but not the green frame).

I could then do the rest with the existing override mechanism. (Though, thinking whether it would make sense to define a config variable with a name of an extra CSS file to load, avoiding creation of a custom template if the only thing changing is extra CSS ... how would that sound to you?)

Cheers, Vlad

PS: Agree about dropping translations from the page - and yes, you are right, we are running our site as English-only.

zmousm commented 5 years ago

Hi @vladimir-mencl-eresearch

This is how eduroam.gr looks like without the border:

eduroam.gr/connect

I admit it's not bad but I am really not excited about the icon:

Let me go back to provide some perspective for the design:

In this respect

We could have a setting (of sorts) to enable showing the "CAT-enabled" icon, but let's be frank, we are clearly stepping into settings bloat territory here -- understanding this customization complexity quite some time ago re-enforced my belief that a separate, replaceable front-end app is the way to go in the long term. We could even have it always on; I would not object if you insist, provided we agree on a suitable icon. However I would suggest that you keep the tweak via a custom template for some time (a couple of months) to see how you like it by then, while we address other matters (such as eduroam db v2), and maybe also have time to think/discuss about the (missing) complementary part I mentioned previously.

I feel that discussing such matters via other means beforehand might be useful, if only for saving some time.

vladimir-mencl-eresearch commented 5 years ago

Hi @zmousm , I take your points - about clean layout and about settings bloat. I'll solve this with a local customized template - thanks again for your input here. I'll close this PR now - thanks again for the feedback. Cheers, Vlad

zmousm commented 5 years ago

@vladimir-mencl-eresearch do you not want to merge this patch so that you can override the institution selector block?

--- a/djnro/templates/front/connect.html
+++ b/djnro/templates/front/connect.html
@@ -68,6 +68,7 @@
        {% for i in institutions %}
        {% with catidp=institutions_cat|dict_lookup:i.institution.pk %}
        <li>
+         {% block cat_institution_selector %}
          {% if catidp and catexists %}
          <a class="btn" data-toggle="modal" data-target="catModal"
             data-idu="{% url 'participants' %}"
@@ -78,6 +79,7 @@
          {% endif %}
          <span class="title" {% if i.oper_name %}data-on="{{i.oper_name}}"{% endif %}>{% tolocale i.institution LANGUAGE_CODE %}</span><span class="small-small hidden">&nbsp;<sp
          </a>
+         {% endblock %}
        </li>
        {% endwith %}
        {% endfor %}