UX improvements

In this patch:

• moves salesforce_api_settings_form into salesforce_api.admin.inc -- the hook_menu already includes this file, so let's put it there
• implements hook_requirements -- status info on admin/reports/status
• implements hook_enable -- prompts user to enter credentials at admin/settings/salesforce
• invalidates salesforce_api_settings_form on connection error -- prevents user from saving invalid credentials and from overwriting valid credentials
• changes salesforce_api_password field on salesforce_api_settings_form from "password" to "textfield" -- because that has always bugged me

Attachment	Size
442678-ux-improvements.patch	10.68 KB

Wow! That was fast! I'll review this later. :)

This needs a reroll; Moving the admin settings form into salesforce_api.admin.inc has already been committed elsewhere. Several whitespace changes have also been committed. There are some issues about some of the changes:

Check out the code from CVS so that your patches are cleaner and don't include additions that drupal.org's packager makes to *.info files in the downloadable tarballs.

Setting then restoring salesforce_api variables is a hack, and prone to error, for example if the toolkit is unavailable. Instead, modify salesforce_api_connect() to take connection settings as an optional parameter. If the parameter has not been passed then salesforce_api_connect() should get the settings from variable_get(). Make the optional parameter NULL by default, but pass it an array, so that the function's signature and function documentation doesn't get ugly.

You'll need to re-add $form['#validate'][] = 'salesforce_api_settings_form_validate'; back into the settings form and patch.

salesforce_api_enable() is mostly good, though salesforce module usually refers to "Salesforce", not "SalesForce", and we should be consistent on this. We should address this issue too, though perhaps in a separate issue and patch.

The method you have used for t() doesn't allow the link text to be translated. For better i18n-compatibility t() should take a constant string as it's parameter, not a variable. The words "click here" are meaningless in links.

Suggestion to solve the above issues; drupal_set_message(t('Salesforce module requires you <a href="!url">enter your Salesforce connection details</a> before it can connect to Salesforce.com.', $vars));

salesforce_api_requirements() looks, though I didn't run it. I'm not sure if curly braces in switch commands is accepted in Drupal's coding standards, though I think they should be! What a great idea! I wouldn't use compact() for something like this since it adds unnecessary vagueness without saving many lines of code, which makes it harder for less-experienced programmers to read.

finally got around to cleaning this up.

Bevan,
I rerolled against 6.x-2.x-dev and implemented all the changes you suggested, except for the t() syntax.
Using <a href> inside t() creates more problems than it solves if, e.g. the user doesn't have clean urls or has set up url aliases...
here's what i'm using instead:
drupal_set_message(t('Salesforce module enabled: Before making any Salesforce connections, !link', array('!link' => l(t('please enter your Salesforce API credentials'), SALESFORCE_PATH_ADMIN))), 'warning');

Also, I added 2 more minor things:

• All the Salesforce config fields (username, password, token) are required (since sf_api requires them to login!)
• Description on password field.

Attachment	Size
442678-ux-improvements.diff	6.94 KB

Links, clean urls, and aliases, do not cause problems because url() handles this at render-time taking all of that into consideration. !url gets substituted for url()'s return value at render-time too. In fact <a href="http://api.drupal.org/api/function/l/6">l()</a> uses url() to generate the URI that l() puts in <a href="">.

The patch still needs review, though feel free to correct this and re-submit.

Sorry, I'm still not clear on the value of using an HTML <a> tag and url() function, when as you noted l() does the same thing.

url() and l() are not the same. l() is dependent on url()

url() returns a uri, e.g. http://drupal.org/node/12345
l() returns an html link, e.g.<a href="http://drupal.org/node/12345">Some node</a>

In order to make strings translatable one whole conmprehensible and meaningful phrases, including the parts inside and outside the <a>, we call t() ONCE with the whole phrase, and include minimal amounts of html in that translatable string.

Imagine trying to translate "Salesforce module requires you !link before it can connect to Salesforce.com." and "enter your Salesforce connection details" and as 2 disconnected and separate language strings, without the context of each other. Translation becomes difficult and the translation is likely to lose it's meaning.

I see your point, Bevan.
Both solutions still seem less than optimal to me though... translation is hard =P

Translation IS hard.

I'm not sure what is sub-optimal about

though? Because the translator has to look at some html? The translator MUST decide which text goes inside and outside the link – so there has to be SOME kind of placeholder. Arguably it could be simpler than <a href="!url">, though I don't think that is very complicated. If it were <em> instead I think it would be fine. Anything more complicated than one or two trivial attributes would probably start to be a problem though. In that case you could also do

But that quite quickly starts to get more complicated in the PHP than necessary.

Fixed a simpler version in http://drupal.org/cvs?commit=245274

Attachment	Size
442678-ux-improvements-2.patch	4.35 KB

Automatically closed -- issue fixed for 2 weeks with no activity.