Open terms in new window

Ahhh exactly what I was looking for! I have the same situation with a long application form and did not want to frustrate the user with having to fill in the form again. Thanks srlawr

The same place, but:

        $checkbox_label = str_replace('@link', l($node->title, 'node/' . $node->nid, array('attributes' => array('rel'=>'shadowbox;'), 'absolute'=>TRUE)), $checkbox_label);

This will show Terms of Use in Shadowbox window, without refreshing current page.

Also I am create bug for wrapping this into theme, not patching code.

I think it would be best to make an admin UI option to select the target as well as have the label themeable as per #744800: Separate checkbox_label generation into theme function

Here's a simple patch which adds Colorbox integration. It would be great if a number of these were options on an admin screen. For instance, this patch has no dimensions in the url query, so the Colorbox opens in the full window (obviously, it will need changing for other use cases). This patch doesn't check for the existence of the Colorbox module first, but it shouldn't be harmful since it only adds a CSS class. To make it work, you'll need to visit the Colorbox configuration screen add check the option for 'Enable Colorbox load'.

oops, here's the same patch without the full urls...

The issue with the patch in #5 is it forces colorbox.
If it is to be included it should be an option that someone can select in the admin screen.

Hi

maybe we can just add a configuration option to add a css class to the anchor?

The patch above is fixed to colorbox and that is not what we really desire.

Unassigning the task.

I used the same method as #2, but giving the link the relationship 'external' and using jQuery to open it in a new window. rel="external" is also xhtml valid, fwiw.

$checkbox_label = str_replace('@link', l($node->title, 'node/' . $node->nid, array('attributes' => array('rel'=>'external'), 'absolute'=>TRUE)), $checkbox_label);

// external sites in new window
$('a[rel*=external]').click( function() {
    window.open(this.href);
    return false;
});

The backend setting solution would be better, of course.

I'm throwing my support behind #7. Allowing us to add a class to the link makes colorbox, new window, and countless other things easier without forcing anything or adding much weight to the module.

The #7 suggestion is also surprisingly similar to the suggestion in #6 :)

Just thought you meant to be enable/disable colorbox. There's a lot more functionality than just that enabling classes would allow. But yeah same idea, don't force anything but we need a way to extend, class does this nicely imo.

Here's a simple patch based on #5, but adding admin controls as per #6 & #7.

Based on 7.x-1.2

The patch in #13 is more complete, but does not work with Shadowbox. Shadowbox need the attribute "rel".

The patch in #2 opens the Shadowbox, but the node of terms aren't opened in the modal window (in the finish of loading, the modal window is closed and the page opens normally in the browser).

I think it would be interesting to insert in the patch at #13 options for the use of the attributes "css" or "rel", which would expand the use of other modal modules.

I disagree with #14. It creates a reliance or an expectation that the Shadowbox module will be enabled. The idea of adding in the class should allow themes to bring up the text in another manner. In this instance I think a hook_form_alter() in a custom module might be the best for your unique usecase.

#13 is almost RTBC, however I have a couple of additions. If these are rolled in, then I'll happily RTBC it. If I make the additions I won't be able to do that.

1. +++ b/sites/all/modules/terms_of_use/terms_of_use.admin.inc
   @@ -50,6 +50,24 @@ function terms_of_use_admin_settings() {
   +    '#description' => t('Add additional classes to the link item here (e.g. "colorbox-load", etc)'),
   

   It would be good to specify that the classes should be space delimited if using more than one. We also don't need () if using e.g.

2. +++ b/sites/all/modules/terms_of_use/terms_of_use.admin.inc
   @@ -50,6 +50,24 @@ function terms_of_use_admin_settings() {
   +      '_self' => 'Same frame (default)',
   

   Do we need the (default) here?

3. +++ b/sites/all/modules/terms_of_use/terms_of_use.admin.inc
   @@ -50,6 +50,24 @@ function terms_of_use_admin_settings() {
   +      '_blank' => 'New Window',
   

   window shouldn't be capitalised

4. +++ b/sites/all/modules/terms_of_use/terms_of_use.admin.inc
   @@ -50,6 +50,24 @@ function terms_of_use_admin_settings() {
   +    '#description' => t('Link target value'),
   

   This could be made more descriptive. 'Target value to be added to the link e.g. target="_blank"'

5. +++ b/sites/all/modules/terms_of_use/terms_of_use.module
   @@ -73,7 +73,7 @@ function terms_of_use_form_user_register_form_alter(&$form, $form_state) {
   +        $checkbox_label = str_replace('@link', l($node->title, 'node/' . $node->nid, array('attributes' => array('class' => array(variable_get('terms_of_use_link_classes', '')), 'target' => array(variable_get('terms_of_use_link_target', '_self'))))), $checkbox_label);
   

   For code readability, I think it might be better to assign these two variable_get() calls to variables and put them in the l() function.

Additionally, we should add variable_del() calls to terms_of_use.install for the new variables introduced in this patch.

Please review.

Nice work @rpayanm, thank you.

Any progress on this?

Hi

can you backup this patch that it is okay? I can commit it to the git but currently not test it...

Tested and it works as seen in the attached screenshots.

Okay there where some very minrot things code wise as missing "," at last array items. I made a new patch to try if the automated test will work. If yes I will commit the patch. :)

Next try

dev version...

And again without color.

Would be great if somebody fixes the test and adds a test if the selectbox for the link type works.

Needs to reroll the patch against the latest dev

Closed due to inactivity. If it is relevant, we will open it again