The project is now known as Parrot.

Parrot sits on the shoulders of some great projects. Parrot is a Drupal 7 Mothership (http://drupal.org/project/mothership) subtheme that uses "some" Bootstrap CSS structure, with SASS & Compass. The theme takes the work the mortendk has done with Mothership, and adds an additional CSS structure that allows a standard to be followed for theme development in a team setting.

The SASS structure is setup to standardize the location of CSS markup, based on the component, not the page content, or path. For example, a page is made up with type, images, and maybe a slideshow, each style for the elements would be placed in their own specified place in the SASS structure, allowing someone from, another team, company, or what have you, go into the theme, and be able to find the style easily when the structure is known.

Structure in the CSS is something that has increased the quality of the theming output, code wise, at ImageX Media, especially when working with client teams, or even contractor developers that we use from time to time.

Project Info

Drupal version: 7.x
Project page: http://drupal.org/sandbox/bmx269/1848916
Git repository: git clone --recursive --branch 7.x-1.x http://git.drupal.org/sandbox/bmx269/1848916.git parrot

Comments

Status:Needs review» Needs work

Hi

Some drupal standars missing. plz see here:
http://ventral.org/pareview/httpgitdrupalorgsandboxbmx2691848916git

Renamed the README.md to README.txt as per the standard.

The updated review does not show errors, except that there is not a release branch now. Thanks

http://ventral.org/pareview/httpgitdrupalorgsandboxbmx2691848916git

Fixed git branch naming.

git clone --recursive --branch 7.x-1.x http://git.drupal.org/sandbox/bmx269/1848916.git mothership_strappy
cd mothership_strappy

Issue summary:View changes

Updating format of project info

You need to set the status to "needs review" if you want to get a review, see http://drupal.org/node/532400

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 :-)

Status:Needs work» Needs review

Updated status to Needs Review.

@klausi I appreciate you are busy, I will try and review some other projects as time allows.

Issue summary:View changes

Fixed branch naming

Changed the project name to Parrot, to be easier to remember, and better to brand.

Priority:Normal» Critical
Status:Needs review» Needs work

Please place your theme files with README.txt folder.

Your current structure is like:

/README.txt
/parrot/parrot.info
/parrot/*

while it should be like this:

/README.txt
/parrot.info
/*

Priority:Critical» Normal
Status:Needs work» Needs review

The file structure has been updated. Thanks.

Naming conventions for preprocess functions in template.php is not as per Drupal standard.

Your current code is like:

function NEWTHEME_preprocess_node(&$vars,$hook)

while it should be like this:

function parrot_preprocess_node(&$vars,$hook)

Change the name of folder from "img" to "images" in theme directory.

img vs images? is there a drupal standard for this?

Fixed the NEWTHEME error. Thanks

It would be better if you used images instead of img. If you look at all other themes in drupal they used images instead of img.

I would suggest:

themename
-fonts
-images
-js
-templates
-css
themename.info
templates.php

As the structure for this theme has started from Twitter Bootstrap, which uses the folder /img for images, I will be using /img naming convention in this case. Thanks for the suggestion.

Any other feedback on the project?

Code Issues:

In your README.txt you have a section on renaming the theme after installing it. It would be advisable to mention renaming the function names in template.php to reflect the new machine name being used for the theme.

Licensing Issues:

The font-awesome font is licensed under the SIL Open Font License. Drupal.org only allows GPL licensed code afaik. You will likely have to remove the font from the theme and have users install it manually unfortunately, however you may wish to wait for confirmation on this.

(See: http://fortawesome.github.com/Font-Awesome/#license for more information on Font-Awesome licensing)

The other licensing issue may be that bootstrap itself is Apache 2.0, again, not GPL. Sorry to throw these out there but I'm not certain just how absolute the GPL-only requirement is.

(See: http://twitter.github.com/bootstrap/ bottom of page, for Bootstrap licensing)

Automated Review:

Automated review returns no issues.

Status:Needs review» Needs work

Changing status to needs work due to licensing issue.

Thanks for the review, The Bootstrap is not added as is, it is heavily modified, and partially used for it's file structure. As for the Font Awesome, I can remove that if there is a conflict with the Licenses.

After my own research it seems that the bootstrap code being Apache 2 does not need to be licensed under an apache2 license. I would imagine that the fact that the code can be 're-licensed' in a derivative changes things quite significantly as the bootstrap code, or whatever part remains, can be re-licensed GPLv2+

More research suggests that inclusion of the font would not be compatible with the agreement accepted when signing up for git access to only keep GLPv2+ code in repos. Due to the license terms applied to various components of the font.

That is what I thought, I will have to remove the Font Awesome part though.

You can potentially substitute the font with a GPL licensed font... Google Web Fonts have some AFAIK.

Font Awesome is an icon font. I will look for an alternative. For now I am pushing up a version with it removed.

Status:Needs work» Needs review
  • Readme.txt Updated
  • Font Awesome Removed

Thanks

Any other feedback?

@bmx269 -- It got a bit buried here but have a look comment #5, it would probably help greatly if you took advantage of the Review Bonus program. Once you do that, then you can tag this issue and get it bumped for more eyes to see and hopefully finalize.

Hello,

  • Line 51 of page.tpl.php should be instead of (remove hash tag)
  • Why is there a _reset.scss when you can 1. add a reset.css through the Mothership settings or 2. include a reset css through compass (@import "compass/reset")
  • I see that you use pixel values a lot when it comes to margins and paddings. Have you thought about using vertical rhythm (http://compass-style.org/reference/compass/typography/vertical_rhythm/)? It would make more sense when it comes to a responsive layout. Also it's just awesomely easy to use :)
  • Just a suggestion: have you considered using png sprites? (http://compass-style.org/help/tutorials/spriting/)
  • Something I've picked up from reading other reviews: you wanna get rid of that .gitignore file in your root folder
  • This is just nitpicking but in template hooks $vars should be $variables, according to Drupal standards.

Other than that it looks awesome. Drupal needs more clean starting themes, and anything building on Mothership should be encouraged. By the way, if you haven't already, check out Display Suite (http://drupal.org/project/ds), I find it to be a very powerful tool (especially in combination with Mothership) to obtain clean markup.

It's great to see that somebody is investing time in creating a system and structure for CSS. I wonder where would you put blocks and views? For example if you have news block. Is it a component in a _news.scss? How would you differ frontpage news, sidebar news nad news detail page?

0.
I like the base folder and that there are _mixins.scss, _reset.scss and _variables.scss inside it. But why do you use !important on allmost all of the variables? It's messy.

1.
Why do you have all the scripts but don't use them anywhere? For including manually later?

2.
Script.js is included but the content is commented out. Why? Also shouldn't there be $().ready( inside the anonymous function for quick start? (just a tip :) )

3.
A minor but still a problem. Comments start with a capital letter and end with a period (.).
http://drupal.org/node/1354
http://drupal.org/coding-standards#comment
So instead of
//this is the default number of columns
One would write
// This is the default number of columns.

And it just disturbs a bit :)

4.
Readme should wrap at 80 chars ( http://drupal.org/node/447604 )

Cheers!

Thanks for reviewing the theme guys.

@samspinoy
There is a reset included as it contains all the needed resets for Bootstrap, as well as some Drupal specific ones. I am not a fan of trusting compass imports for all code. I even considered including the source for the grid systems, but was convinced to try the compass way. There are reasons, like being able to see all css before compile, and more which could take a long time to talk about.

Vertical rhythm is awesome, but I did not want to spend the time on this release to go through the theme's styles to get it set for that. Maybe in version 2.

.gitignore is not allowed? Strange.

I will look into the $vars issue. Thanks for the reviewing.

@hkirsman
The _variables.scss is based on Bootstrap, that is where that is coming from.
I will have to change all the comments from Bootstap to follow Drupal coding standards.

I will update the code to follow the standards elsewhere as well.

Upon entering this page I received these errors admin/appearance/settings/parrot

Notice: Undefined property: stdClass::$prefix in system_theme_settings() (line 547 of /var/www/test.com/modules/system/system.admin.inc).
Notice: Undefined index: mothership in system_theme_settings() (line 575 of /var/www/test.com/modules/system/system.admin.inc).
Notice: Trying to get property of non-object in system_theme_settings() (line 575 of /var/www/test.com/modules/system/system.admin.inc).

front page

Notice: Undefined index: mothership in drupal_theme_initialize() (line 98 of /var/www/test.com/includes/theme.inc).
Notice: Trying to get property of non-object in drupal_alter() (line 1042 of /var/www/test.com/includes/module.inc).
Notice: Trying to get property of non-object in drupal_alter() (line 1042 of /var/www/test.com/includes/module.inc).
Notice: Trying to get property of non-object in phptemplate_init() (line 12 of /var/www/test.com/themes/engines/phptemplate/phptemplate.engine).
Notice: Trying to get property of non-object in drupal_alter() (line 1042 of /var/www/test.com/includes/module.inc).
Notice: Trying to get property of non-object in drupal_alter() (line 1042 of /var/www/test.com/includes/module.inc).
Notice: Trying to get property of non-object in drupal_alter() (line 1042 of /var/www/test.com/includes/module.inc).
Notice: Trying to get property of non-object in drupal_alter() (line 1042 of /var/www/test.com/includes/module.inc).
Notice: Trying to get property of non-object in _theme_build_registry() (line 700 of /var/www/test.com/includes/theme.inc).
Notice: Trying to get property of non-object in _theme_build_registry() (line 702 of /var/www/test.com/includes/theme.inc).
Notice: Trying to get property of non-object in _theme_build_registry() (line 704 of /var/www/test.com/includes/theme.inc).
Notice: Trying to get property of non-object in _theme_build_registry() (line 704 of /var/www/test.com/includes/theme.inc).
Notice: Trying to get property of non-object in drupal_alter() (line 1042 of /var/www/test.com/includes/module.inc).
Notice: Trying to get property of non-object in drupal_alter() (line 1042 of /var/www/test.com/includes/module.inc).
Notice: Trying to get property of non-object in drupal_alter() (line 1042 of /var/www/test.com/includes/module.inc).
Notice: Trying to get property of non-object in drupal_alter() (line 1042 of /var/www/test.com/includes/module.inc).
Notice: Trying to get property of non-object in drupal_alter() (line 1042 of /var/www/test.com/includes/module.inc).
Notice: Trying to get property of non-object in drupal_alter() (line 1042 of /var/www/test.com/includes/module.inc).
Notice: Trying to get property of non-object in drupal_alter() (line 1042 of /var/www/test.com/includes/module.inc).
Notice: Trying to get property of non-object in drupal_alter() (line 1042 of /var/www/test.com/includes/module.inc).
Notice: Trying to get property of non-object in drupal_alter() (line 1042 of /var/www/test.com/includes/module.inc).
Notice: Trying to get property of non-object in drupal_alter() (line 1042 of /var/www/test.com/includes/module.inc).
Notice: Trying to get property of non-object in drupal_alter() (line 1042 of /var/www/test.com/includes/module.inc).
Notice: Trying to get property of non-object in drupal_alter() (line 1042 of /var/www/test.com/includes/module.inc).
Notice: Trying to get property of non-object in drupal_alter() (line 1042 of /var/www/test.com/includes/module.inc).
Notice: Trying to get property of non-object in drupal_alter() (line 1042 of /var/www/test.com/includes/module.inc).
Notice: Trying to get property of non-object in drupal_alter() (line 1042 of /var/www/test.com/includes/module.inc).
Notice: Trying to get property of non-object in drupal_alter() (line 1042 of /var/www/test.com/includes/module.inc).
Notice: Trying to get property of non-object in drupal_alter() (line 1042 of /var/www/test.com/includes/module.inc).
Notice: Trying to get property of non-object in drupal_alter() (line 1042 of /var/www/test.com/includes/module.inc).

Looks like you need some work. Maybe you didn't "git push" your current work.

Do you have the release Mothership base theme installed? We have used this theme on at least 6 large sites with no issues.

Sorry. Had to clear the cache a few times and now it is working. I see you spent a lot of time. It looks good to me. No errors now.

Updated to use Compass/Reset, updated panels templates, bugfixes.

Title:Mothership Strappy[D7] Parrot
Status:Needs review» Needs work

You should remove the .gitignore file, config.rb, and js/ (since script.js is empty) from your repo.

template.php is a mess - the comments are useless and need improvements. Remove all the commented-out code and please follow the standards for Drupal comments (a space after //, start with a capital letter, and end in a period).

If you can fix the extraneous files you'll get an RTBC from me.

Made the following changes:

  • Updated comments in template.php.
  • Removed config.rb; added instructions on project page, as well in the README.txt, about creating the required config.rb file.
  • Removed empty js/scripts.js file.
  • Updated favicon and logo file.
  • Removed .gitignore.

I hope this helps the approval process. We have used the theme in multiple projects with great results.

Status:Needs work» Needs review

Updating status to needs review.

Status:Needs review» Reviewed & tested by the community

Guess I'm not up on what config.rb does, but if it's required for your theme to work then it's fine, sorry! I thought it was some leftover ruby config file.
----
Top Shelf Modules - Enterprise modules from the community for the community.

It is used for Compass, a Ruby based sass addon. It is used to provide extra benefits to the sass language. The readme tells all that is needed, if there are issues with it missing in the issue queue, I will add it back into the theme. Thanks for reviewing.

Status:Reviewed & tested by the community» Closed (duplicate)
Multiple Applications
It appears that there have been multiple project applications opened under your username:

Project 1: http://drupal.org/node/2008584

Project 2: http://drupal.org/node/1891548

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

I'm a robot and this is an automated message from Project Applications Scraper.

Status:Closed (duplicate)» Reviewed & tested by the community

Reopened

Am I now just waiting on the Git moderator approval? No rush, just curious as this is my first contrib project.

You probably need this set back to 'needs review' just to get the attention back on it.

I am confused as to why I would want to have it needs review, when it was reviewed?

Just because the project reviewing team are looking for projects marked as needs review. Which means they should review this.

As I said - I'm not certain but I suspect it may help.

leave it in RTBC status - that should get the attention of the queue admins.

Status:Reviewed & tested by the community» Fixed

You don't need to declare php = 5.2 in your .info file, Drupal 7 already requires PHP 5.2.5. Code still looks good and it's been over a month.

Thanks for your contribution, bmx269!

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.

----
Top Shelf Modules - Enterprise modules from the community for the community.

Awesome. Thanks Guys.

-Trent

Status:Fixed» Closed (fixed)

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

Issue summary:View changes

Changed project name to be Parrot