Popup Views integration [D7]

hello

Ventral (ventral.org) reports some issues with coding standards - http://ventral.org/pareview/httpgitdrupalorgsandboxeme1596128git

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.

• README.txt is missing, see the guidelines for in-project documentation.
• Remove "version" from the info file, it will be added by drupal.org packaging automatically.
• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

You can use ventral quickly to test a git on drupal, or download the code review module, or drupal code sniffer so you can work on coding standards locally.

Also use "needs review" to get people to review your code :)

Hope this helps

* @file block missing in popup_views_integration.views.inc file refer Drupal Docs
* Readme.txt is missing
* Function comment formatting
* See the attached file to see PAReview results

Maybe you can add some document, like summary, system requirements, how to install, how to configure, customization. For example syscrusher style.

And still there some errors when i do automated project review.

FILE: ...iew_temp/test_candidate/popup_views_integration_handler_field_popup.inc
--------------------------------------------------------------------------------
FOUND 11 ERROR(S) AFFECTING 5 LINE(S)
--------------------------------------------------------------------------------
11 | ERROR | Class name must begin with a capital letter
11 | ERROR | Class name must use UpperCamel naming without underscores
12 | ERROR | Missing function doc comment
12 | ERROR | No scope modifier specified for function "query"
17 | ERROR | Missing function doc comment
17 | ERROR | Method name
| | "popup_views_integration_handler_field_popup::option_definition"
| | is not in lowerCamel format, it must not contain underscores
17 | ERROR | No scope modifier specified for function "option_definition"
36 | ERROR | Missing function doc comment
36 | ERROR | Method name
| | "popup_views_integration_handler_field_popup::options_form" is
| | not in lowerCamel format, it must not contain underscores
36 | ERROR | No scope modifier specified for function "options_form"
136 | ERROR | No scope modifier specified for function "render"
--------------------------------------------------------------------------------

Answer your question:
Maybe because you set the Identity of Git for this sandbox to the "Bryvice", remembering you have two user id, "eme" and "Bryvice". So you post the sandbox of your project via Bryvice, not the eme ones.

Hi eme,

I did a manual review of your code and found couple of issues.

In the file popup_views_integration_handler_field_popup.inc on line 43 i can see that you have used '#options' for a form item which is of type 'textarea' which is incorrect. Also the $fields is not defined.

In the same file i see that you have not commented (function doc comment) for some functions, its better you write a comment for each function in this file before defining the function. Its also better you provide scope modifier for each function within class. This will reduce the error count on ventral.org

Regards
M

I've only tested the code but it looks to work and solved my issue with fivestar in combination with VBO. Great!

First of all, awesome what you have done here!

I created a views table with a few pop-up fields, no problem.

Now I want the hover link to be an image, i tried adding <img src="myfile.png"> to the link area but this doesn't work, it just shows the default text.

The pop-up link field description tells me: Combine tokens from the "Replacement patterns" and html to create the trigger link. But the html doesn't seem to work, also adding an image to a custom field and placing the token in the link field doesn't work.

Any ideas?

I noticed this line in popup_views_integration_handler_field_popup.inc:

//Filter links because it breaks the popup.

What do you mean by that?
Because I need the functionality without the regular expression in title so that it can use normal HTML tags. But does this break somewhere? I haven't noticed it...
Or is there another way to use HTML in the link generation field?

Hi, nice module, great work!

Have you contacted the maintainers of the popup module? Perhaps views integration could be incorporated into the popup module rather than being a separate project.

To me the code looks fine. There are just a few small things I think should be fixed:

1. As per #14, the description for the link filed says "The text to display for this field. You may include HTML. You may enter data from this view as per the "Replacement patterns" below." . However it appears HTML is filtered out of this field in line 151 of popup_views_integration_handler_field_popup.inc. You should either change the description or allow HTML.

2.The Link field has the title "Link" with instructions on filling it out in the description attribute. However the field below that "Text to display" shows the instructions as part of the link title. You should put those instructions in the description attribute for consistency.

3. I wonder whether it would be better UX to change the wording of the descriptions of these fields to more closely follow the descriptions in other parts of the views UI. The 'rewrite the output of this field' text area in views core has the following description: "The text to display for this field. You may include HTML. You may enter data from this view as per the "Replacement patterns" below." This can be re-used for your fields with a couple of small changes, which would help users who are accustomed to the core views UI.

4. popup_views_integration_handler_field_popup.inc line 6:
* A handler to provide a field that is completely custom by the administrator.
Is this description left over from another file? It doesn't seem to address the field handler in question.

5. popup_views_integration_handler_field_popup.inc line 79:
'#description' => t('Only work with "On click" activation mode'),
Should be 'Only works with "On click" activation mode.'

Hi eme,

It looks like you added your last commit to your master branch. You should remove your master branch and make commits to the 7.x-1.x branch, see

http://drupal.org/node/1127732

Let me know when this is done and I'll review the fixes in #17.

Much better descriptions on the form. Some errors have crept in however. Did you test the patch before committing?

1.Sytax error - line 58 of popup_views_integration_handler_field_popup.inc ends in a period, should be a comma.

This throws an ajax error when you try to add a popup field to view.

2.Viewing a popup view page is throwing notices:

Notice: Undefined index: path in popup_views_integration_handler_field_popup->render() (line 157 of /Users/phildodd/Sites/d7reviews/sites/all/modules/popup_views_integration/popup_views_integration_handler_field_popup.inc).

It looks like the 'path' option is not defined in the config form..?

Hello eme,

I am really interested with your module, It is super cool.

I test it and, It is working well for me.

You still have some small corrections to do in 2 files in your module :

FILE: /var/www/drupal-7-pareview/pareview_temp/README.txt
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AND 4 WARNING(S) AFFECTING 5 LINE(S)
--------------------------------------------------------------------------------
1 | WARNING | Line exceeds 80 characters; contains 116 characters
3 | WARNING | Line exceeds 80 characters; contains 123 characters
5 | WARNING | Line exceeds 80 characters; contains 240 characters
20 | WARNING | Line exceeds 80 characters; contains 82 characters
24 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

and

FILE: ...-pareview/pareview_temp/popup_views_integration_handler_field_popup.inc
--------------------------------------------------------------------------------
FOUND 33 ERROR(S) AND 1 WARNING(S) AFFECTING 22 LINE(S)
--------------------------------------------------------------------------------
6 | WARNING | Line exceeds 80 characters; contains 89 characters
11 | ERROR | Class name must begin with a capital letter
11 | ERROR | Class name must use UpperCamel naming without underscores
13 | ERROR | Expected 3 space(s) before asterisk; 2 found
14 | ERROR | Expected 3 space(s) before asterisk; 2 found
15 | ERROR | No scope modifier specified for function "query"
20 | ERROR | Spaces must be used to indent lines; tabs are not allowed
20 | ERROR | Line indented incorrectly; expected 2 spaces, found 1
23 | ERROR | Method name
| | "popup_views_integration_handler_field_popup::option_definition"
| | is not in lowerCamel format, it must not contain underscores
23 | ERROR | No scope modifier specified for function "option_definition"
42 | ERROR | Spaces must be used to indent lines; tabs are not allowed
42 | ERROR | Whitespace found at end of line
44 | ERROR | Expected 3 space(s) before asterisk; 2 found
45 | ERROR | Expected 3 space(s) before asterisk; 2 found
46 | ERROR | Method name
| | "popup_views_integration_handler_field_popup::options_form" is
| | not in lowerCamel format, it must not contain underscores
46 | ERROR | No scope modifier specified for function "options_form"
58 | ERROR | Whitespace found at end of line
98 | ERROR | Spaces must be used to indent lines; tabs are not allowed
98 | ERROR | Whitespace found at end of line
159 | ERROR | No scope modifier specified for function "render"
163 | ERROR | Spaces must be used to indent lines; tabs are not allowed
163 | ERROR | Whitespace found at end of line
172 | ERROR | Spaces must be used to indent lines; tabs are not allowed
172 | ERROR | Line indented incorrectly; expected 6 spaces, found 3
174 | ERROR | Spaces must be used to indent lines; tabs are not allowed
174 | ERROR | Whitespace found at end of line
175 | ERROR | Spaces must be used to indent lines; tabs are not allowed
175 | ERROR | Line indented incorrectly; expected 6 spaces, found 3
179 | ERROR | Spaces must be used to indent lines; tabs are not allowed
179 | ERROR | Whitespace found at end of line
181 | ERROR | Inline control structures are not allowed
183 | ERROR | Spaces must be used to indent lines; tabs are not allowed
183 | ERROR | Whitespace found at end of line
187 | ERROR | Inline control structures are not allowed
--------------------------------------------------------------------------------

Do take time to rectify the issues.
Your module is almost ready to get a RTBC

Hi eme,

It's looking good. Unfortunately I broke it. I was testing sanitsation by entering

into the various fields, after saving I can no longer edit the popup settings - I click on "global:popup" in the fields, the configuration overlay does not load. I'm guessing at least one of your fields is not sanitising properly.

Here is my view. It's on a plain install of drupal with some devel-generated content, so should be pretty easy to import and test.

$view = new view();
$view->name = 'popup2';
$view->description = '';
$view->tag = 'default';
$view->base_table = 'node';
$view->human_name = 'popup2';
$view->core = 7;
$view->api_version = '3.0';
$view->disabled = FALSE; /* Edit this to true to make a default view disabled initially */

/* Display: Master */
$handler = $view->new_display('default', 'Master', 'default');
$handler->display->display_options['title'] = 'popup2';
$handler->display->display_options['use_more_always'] = FALSE;
$handler->display->display_options['access']['type'] = 'perm';
$handler->display->display_options['cache']['type'] = 'none';
$handler->display->display_options['query']['type'] = 'views_query';
$handler->display->display_options['exposed_form']['type'] = 'basic';
$handler->display->display_options['pager']['type'] = 'some';
$handler->display->display_options['pager']['options']['items_per_page'] = '4';
$handler->display->display_options['pager']['options']['offset'] = '0';
$handler->display->display_options['style_plugin'] = 'table';
/* Field: Content: Title */
$handler->display->display_options['fields']['title']['id'] = 'title';
$handler->display->display_options['fields']['title']['table'] = 'node';
$handler->display->display_options['fields']['title']['field'] = 'title';
$handler->display->display_options['fields']['title']['label'] = '';
$handler->display->display_options['fields']['title']['alter']['word_boundary'] = FALSE;
$handler->display->display_options['fields']['title']['alter']['ellipsis'] = FALSE;
/* Field: Content: Image */
$handler->display->display_options['fields']['field_image']['id'] = 'field_image';
$handler->display->display_options['fields']['field_image']['table'] = 'field_data_field_image';
$handler->display->display_options['fields']['field_image']['field'] = 'field_image';
/* Field: Global: Popup */
$handler->display->display_options['fields']['popup']['id'] = 'popup';
$handler->display->display_options['fields']['popup']['table'] = 'popup_views_integration';
$handler->display->display_options['fields']['popup']['field'] = 'popup';
$handler->display->display_options['fields']['popup']['alter']['alter_text'] = TRUE;
$handler->display->display_options['fields']['popup']['title'] = '<strong>banana!! <script>alert("aaa");</script></strong>';
$handler->display->display_options['fields']['popup']['text'] = '[title] <script>alert("xxx");</script>';
$handler->display->display_options['fields']['popup']['path'] = '<script>alert("xxx");</script>';
$handler->display->display_options['fields']['popup']['close'] = 0;
$handler->display->display_options['fields']['popup']['width'] = '150';
$handler->display->display_options['fields']['popup']['style'] = '0';
$handler->display->display_options['fields']['popup']['class'] = '<script>alert("yyy");</script>';
/* Sort criterion: Content: Post date */
$handler->display->display_options['sorts']['created']['id'] = 'created';
$handler->display->display_options['sorts']['created']['table'] = 'node';
$handler->display->display_options['sorts']['created']['field'] = 'created';
$handler->display->display_options['sorts']['created']['order'] = 'DESC';
/* Filter criterion: Content: Published */
$handler->display->display_options['filters']['status']['id'] = 'status';
$handler->display->display_options['filters']['status']['table'] = 'node';
$handler->display->display_options['filters']['status']['field'] = 'status';
$handler->display->display_options['filters']['status']['value'] = 1;
$handler->display->display_options['filters']['status']['group'] = 1;
$handler->display->display_options['filters']['status']['expose']['operator'] = FALSE;

/* Display: Page */
$handler = $view->new_display('page', 'Page', 'page');
$handler->display->display_options['defaults']['hide_admin_links'] = FALSE;
$handler->display->display_options['path'] = 'popup2';

Hi eme,

Nice one, working well now and you have addressed everything else I've spotted.

I'm happy to mark this RTBC.

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

It would be nice to have the "Replacement patterns" diplayed on the configuration form.

But otherwise looks good, so ...

Thanks for your contribution, eme!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

Just leave it at "fixed", it will close automatically after 2 weeks.

I also edited your comment to paste in the correct links, you can use the template from http://groups.drupal.org/node/184389 when approving a user.