Sparkling is a clean and simple responsive theme for Drupal 7. The theme reuses a some features from the Skeleton theme but visually it is very different.

Project page: http://drupal.org/sandbox/gilli/1862296

Git: git clone --recursive --branch master gilli@git.drupal.org:sandbox/gilli/1862296.git

Image:
Sparkling image

Demo: http://owwl.net/

CommentFileSizeAuthor
#7 pareview.txt208.39 KBnalan

Comments

mitrpaka’s picture

mitrpaka commented

Manual review:

1. Development still in master branch - Please move sources to 7.x-1.x branch (See http://drupal.org/node/1127732 for further details)
2. README.txt missing - Please add
3. Please remove LICENSE.txt - Will be added automatically once full project access granted
4. Please remove version line from .info file - Will be added automatically
5. sparkling.css header says "Free to use under the MIT license."
- Have you checked that sparkling.css can be distributed along with this module? Provided under GPL?
6. Please add @file info to templates/page.tpl.php file and template.php
- Other templates has "Bartik's theme implementation for comments." - Should those be changed to "Based on Bartik's theme implementation for comments."?

Demo at http://owwl.net/ and theme itself looks nice!

gillisig’s picture

gillisig commented
Status: Needs review » Needs work

Thanks for the tips and mitrpaka, I will fix those things you mentioned then :)

gillisig’s picture

gillisig commented
Status: Needs work » Needs review

Alright, I have addressed all the issues you pointed out to me.

If there is anything else please let me know and thanks a lot for your help so far. :)

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

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

deapge’s picture

deapge commented
Status: Needs review » Needs work

Hello,
I found some issues:
1. In sparkling.info file you must define core,or I can't use this theme.
2. From PAReview (http://ventral.org/pareview/httpgitdrupalorgsandboxgilli1862296git) I got these warnings:
./README.txt: ASCII text, with no line terminators

gillisig’s picture

gillisig commented

Hi deapge,

1. So what exactly should it say? I had version info there but the first commenter, mitrpaka told me I should remove it since it will be added automatically.

2. I guess it is happening because it is one line with no line break. So I don't see why it should matter.

Thanks! :)

nalan’s picture

nalan commented
StatusFileSize
new pareview.txt208.39 KB

Hi,

Manual review:
1) The theme is not supported by drupal 7. Check if u have mentioned the drupal core in your info file.
2) The fonts that you have used... do u have the permission to distribute it?

Automatic review : (7.x-1.x branch)
There are lot of coding style issue to take care of.
* Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
* Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

mayank-kamothi’s picture

mayank-kamothi commented

Hi,

Manual Review

  • In page.tpl.php, node.tpl.php you have curly braces, it would probably be better to use the alternative php syntax.
  • In theme you have provided support for multilingual Menu, so better to provide consistency of same feature support to complete theme :
    Example : use t() for all text information.

Mayank Kamothi

gillisig’s picture

gillisig commented

1) The theme is not supported by drupal 7. Check if u have mentioned the drupal core in your info file.
2) The fonts that you have used... do u have the permission to distribute it?

1) It has been fixed now
2) Yes, they are provided by google fonts so as far as I know i got the permission to user them

There are lot of coding style issue to take care of.
* Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
* Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

Majority of the styling issues are due to me keeping the code in one lines instead of splitting them, which should be a bit better for performance, is that not accepted? I have seen a lot of other themes like this and I can't really see the problem doing it like that.

In page.tpl.php, node.tpl.php you have curly braces, it would probably be better to use the alternative php syntax.
In theme you have provided support for multilingual Menu, so better to provide consistency of same feature support to complete theme :
Example : use t() for all text information.

Why should I not use curly braces? And would that alternative be doing " if (foo < bar): " and then "endif" instead of " if (foo < bar){} " ?
Could you point out where exactly I should use t() since I can't really see any text that would need translation.

Thanks for all the comments guys! :)

- Gilli

PA robot’s picture

PA robot commented
Status: Needs work » Closed (won't fix)

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

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

gillisig’s picture

gillisig commented
Status: Closed (won't fix) » Needs review
kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
commented
Title: Sparkling theme » [D7] Sparkling theme
Status: Needs review » Needs work

You still have far too many errors reported by http://ventral.org/pareview/httpgitdrupalorgsandboxgilli1862296git. Most of them are just whitespace issues.

As to your specific questions:

Why should I not use curly braces [on template files]?

see https://drupal.org/coding-standards#Alternate control statement syntax for templates.

Could you point out where exactly I should use t()

I did not find any instances where you were missing t(). All text appears to be translatable.

Majority of the styling issues are due to me keeping the code in one lines instead of splitting them, which should be a bit better for performance, is that not accepted?

Correct, that does not follow Drupal's CSS coding standards and should be fixed.

The fonts that you have used... do u have the permission to distribute it?

I don't see any fonts included in the code repository, so that should not be an issue.

We have coding standards since it makes it easier for us to share code with each other. Drupal is a very large project with thousands of collaborators around the world, and this ends up being important. You can discuss or make suggestions to the standard in this group: https://groups.drupal.org/coding-standards.

Your demo site looks great, thanks for adding that. If you can fix the whitespace issues you'll get an RTBC from me.

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

gillisig’s picture

gillisig commented

Hi kscheirer,

I fixed all of the whitespace issues, all that is left now are some false negatives involving this:
1089 | ERROR | Each style definition must be on a line by itself
1089 | ERROR | Expected 1 space after colon in style definition; 0 found
1109 | ERROR | Expected exactly one new line after opening brace of class

But these just seem to be due to using comments in the CSS and some gradient hacks for IE like this one:
filter: progid:DXImageTransform.Microsoft.gradient(startColorstr='#e6e6e6', endColorstr='#c9c9c9');

kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
commented

Hmm, I still see a bunch of style and syntax errors at http://ventral.org/pareview/httpgitdrupalorgsandboxgilli1862296git. Also, please set this issue to "needs review" to get more eyes on it.

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

PA robot’s picture

PA robot commented
Status: Needs work » Closed (won't fix)

Closing due to lack of activity. Feel free to reopen if you are still working on this application (see also the project application workflow).

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