Improve UI for multiauth

[GoogleCodeExporter]

Right now the multiauth authentication source displays a list of configured 
authentication sources and let the user choose which one he wants to use. The 
problem is that the labels of the list are the names of the authentication 
sources as defined in config/authsources.php. This is not very good from a 
usability point of view.

I would like to:

 - Use localized labels in this list
 - Mark the <li> elements with specific classes so theme authors can add background images to improve the look'n'feel
 - Do this in a backward compatibility fassion so we don't break existing multiauth setups.

I'll attach a patch that accomplish these goals. It is not complete because not 
all existing authentication sources are supported but I would like to know if 
this approach is valid before adding support for all of them.

What do you think?

Original issue reported on code.google.com by lorenzo....@gmail.com on 6 Aug 2011 at 7:47

Attachments:

• better-ui-for-multiauth.diff

[GoogleCodeExporter]

I agree with your goal, but I don't think adding this as dictionaries in each 
authentication module is the right way to do it. There are two reasons for that:

- I think it should be possible for an administrator to easily add custom text 
to different authsources.

- In some cases, an administrator may want to two different authentication 
sources that use the same authentication module.

I therefore want to propose a different approach - extending the multiauth 
'sources' option to allow per-authsource options to be set. For example, one 
could change it so something like:

    'sources' => array(
        'example-saml' => array(
            'text' => array(
                'en' => 'Log in using an user connected to the ASDF federation',
                'no' => 'Logg inn med en bruker fra ASDF føderasjonen',
            ),
        ),
        'example-admin' => array(
            'text' => array(
                'en' => 'Log in using the administrator-user'
            ),
        ),

The sources list is already loaded in the constructor for the multiauth module. 
It should be possible to extend it to support the new syntax in a 
backwards-compatible way. Just iterate over the list of entries, and transform 
any entries where the key is an integer and the value is a string into an entry 
where the key is the string and the value is an empty array.

Original comment by olavmrk@gmail.com on 9 Aug 2011 at 8:01

[GoogleCodeExporter]

Ok Olav,

I'll update my patch to reflect your opinions, which I agree with.

Original comment by lorenzo....@gmail.com on 9 Aug 2011 at 12:19

[GoogleCodeExporter]

This is a new version of the patch with your approach implemented.

I also added an optional 'css' key to each source so theme author can have it 
available on the template.

Original comment by lorenzo....@gmail.com on 9 Aug 2011 at 3:23

Attachments:

• better-ui-for-multiauth-v2.diff

[GoogleCodeExporter]

This patch is the same as v2 but it has a small optimization: only computes the 
authtype in case it is needed (e.g. no css class is provided in the 
configuration).

I think the code is slightly easier to read now.

Original comment by lorenzo....@gmail.com on 9 Aug 2011 at 3:36

Attachments:

• better-ui-for-multiauth-v3.diff

[GoogleCodeExporter]

Some changes I would like to see before this is committed:

- Configuration parsing/processing should generally be done in the constructor 
for the authsource.

- If you pass always create an entry "$info['text']" that is an array with 
language-code => translation in the constructor, you can just call 
$this->t($source['text']) from the template to get it translated.

- Is the authsource type the best default-value for the css class? I would 
imagine that it would be more useful with the authsource ID instead, since you 
can have multiple authsources of the same type?

- Could you also update multiauth/docs/multiauth.txt?

Original comment by olavmrk@gmail.com on 10 Aug 2011 at 7:12

• Added labels: Type-Patch

[GoogleCodeExporter]

I disagree in the default value for the css class. If you are a theme author 
you will want your theme to work out of the box for as many authsources as 
possible. If you depend on what the administrator is going name the authsource 
id you can not make a reusable UI.

If you are an administrator and you have several authsources of the same type 
either:

 - You don't mind them looking the same as they are the same type. The text will be different thought
 - Or, you can actually make them different by specifying the css attribute in the 'sources' array.

Original comment by lorenzo....@gmail.com on 10 Aug 2011 at 8:07

[GoogleCodeExporter]

OK, I can live with that.

I would however like to see a different minor change: could the option be named 
"css-class" or something like that? When reading "css" I find myself thinking 
about something that goes into the style-attribute rather than the 
class-attribute.

Original comment by olavmrk@gmail.com on 10 Aug 2011 at 8:30

[GoogleCodeExporter]

Updated patch with your latest comments implemented.

Original comment by lorenzo....@gmail.com on 10 Aug 2011 at 11:23

Attachments:

• better-ui-for-multiauth-v4.diff

[GoogleCodeExporter]

A minor bug that should be fixed - when you assign $source to $text, you should 
actually assign array('en'=>$source) to $text.

This is needed to avoid having $this->t() try to look up the authsource id in a 
dictionary file.

Original comment by olavmrk@gmail.com on 10 Aug 2011 at 11:34

[GoogleCodeExporter]

I guess the correct thing to do is not use 'en' but config.language.default.

Original comment by lorenzo....@gmail.com on 10 Aug 2011 at 11:42

[GoogleCodeExporter]

Updated patch

Original comment by lorenzo....@gmail.com on 10 Aug 2011 at 12:00

Attachments:

• better-ui-for-multiauth-v5.diff

[GoogleCodeExporter]

Thanks! I have committed your patch as r2890.

Btw.: The default language code doesn't really matter when there is only a 
single entry in the array passed to the translation function. There are a 
series of "fallbacks" in which entry it chooses, and as a last resort it will 
just pick any entry instead of leaving it blank.

Original comment by olavmrk@gmail.com on 11 Aug 2011 at 7:48

• Changed state: Fixed