Api for integration with ubuntus unity api for drupal7

At the moment provides a hook to add menu links to Unity Hud, and sets the application Icon to the set favicon in drupal.

Includes a submodule to add the basic drupal navigation to the Hud.

more info / screenshot in the sandbox description:

http://drupal.org/sandbox/derEremit/1705882

git clone --recursive --branch 7.x-1.x http://git.drupal.org/sandbox/derEremit/1705882.git unity_webapp_integration
cd unity_webapp_integration

Comments

cthiebault’s picture

cthiebault commented

Automatic review does not give any warning as your code follows Drupal standards :-)

Manual review

  • unity_webapp.module, line 24 & unity_webapp_menu.module, line 21: Variable 'links' might have not been defined
  • you could add phpdoc that says you're implentting hooks, like hook_page_alter(&$page)
patrickd’s picture

patrickd commented

@cthiebault please follow workflow: found major issues -> needs work / found no major issues -> RTBC. Leaving issues at needs review when your finished does not really help applicants.

cthiebault’s picture

cthiebault commented

Sorry for this...
I did not change status as I reviewed the code but did not test it (I'm not running Unity)...

leschekfm’s picture

leschekfm commented
Status: Needs review » Needs work

I have noticed that there is a hardcoded baseurl in unity_webapp.module line 28.
If that part was made dynamic I would call this RTBC.

derEremit’s picture

derEremit commented
Status: Needs work » Needs review

easy fix,
commited,
and thanks for the review...

leschekfm’s picture

leschekfm commented
Status: Needs review » Reviewed & tested by the community

Ah, that was fast :)

Tested the module again and it works.

As I didn't notice any other major issues, I think this is RTBC.

patrickd’s picture

patrickd commented
Status: Reviewed & tested by the community » Fixed
  • /**
    * Insert Javascript into the page.
    */
    function unity_webapp_page_alter(&$page) {

    should be

    /**
    * Implements hook_page_alter().
    *
    * Insert Javascript into the page.
    */
    function unity_webapp_page_alter(&$page) {

  • /**
    * Call hooks to build the links.
    * @return array
    * array with links from called hooks
    */

    should be

    /**
    * Call hooks to build the links.
    *
    * @return array
    * array with links from called hooks
    */

  • Rather than using the $base_root global, use the file_create_url($uri) function.
  • //replace standard drupal favicon with higher quality one
    should look like
    // Replace standard drupal favicon with higher quality one.
    Use a space between // and the comment, begin the comment capitalized and end your comments with . ! or ?
  • Your using a javascript setting that you called "baseurl" but you've only put the hostname in. Note that a real "base url" consists of protocol://host:port/directory.

You should really care a little more about commenting ;) but no blockers for me here, therefore

Thanks for your contribution!

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:

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.

Status: Fixed » Closed (fixed)

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