git repository

git clone http://git.drupal.org/sandbox/themestar/1838172.git drupal_professional_theme

sandbox

http://drupal.org/sandbox/themestar/1838172

CommentFileSizeAuthor
home.png498.34 KBthemestar
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ARUN AK’s picture

Status: Needs review » Needs work

Hi themestar,
to review your project please provide you project's sandbox link and git repository link here.

themestar’s picture

i have added sandbox link and git repository link

themestar’s picture

Status: Needs work » Needs review

Need review

indydas’s picture

Hi,

Nice theme, very intriguing to see the framework behind this.

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.

Also this is the results of your master branch from a code review.

http://ventral.org/pareview/httpgitdrupalorgsandboxthemestar1838172git

I have checked out your code now. Could you explain a bit more as to how the theme will be in HTML5? Will users be presented with markup in the templates or will they need to do the markup themselves. Also are you looking to use media queries and breakpoints in the stylesheets so that the theme is responsive to any device?

Thanks!

indydas’s picture

Issue summary: View changes

git repository

themestar’s picture

Thanks for your feedback,
Fixed all http://ventral.org/ reviewed errors
Created "7.x-1.x" branch
Drupal Professional Theme is not HTML5 and Responsive. FIxed width ( 1000px ). sorry i was mistaken in issue summery

odegard’s picture

Theme looks nice.

The coder module reports issues in the info-file, but they are as far as I can see wrong (they report the same problems with Bartik!).

According to the examples module on js_example the script.js should be changed from this:

jQuery(function($){
  $(document).ready(function() {
    // initialise Superfish
    $('#main-menu ul').superfish({
        animation: {height:'show'}
    });
  });
});

to this:

(function($){
  $(document).ready(function() {
    // initialise Superfish
    $('#main-menu ul').superfish({
        animation: {height:'show'}
    });
  });
})(jQuery);

Other than that, looks good to me!

themestar’s picture

Thanks odegard,
Fixed script.js issue. please check

odegard’s picture

Confirmed that the script is updated.

However, I just noticed that the main body background image body.png is 1.8MB!

themestar’s picture

i will compress the image size

themestar’s picture

Fixed,
Removed heavy image files.

odegard’s picture

Confirmed body.png is removed.

zymphonies-dev’s picture

HI,

Manual Review:
please fix these following UI bugs
1) Give button style cursor:pointer and hover effect.
2) Blog: give little more space between each links.

li a { padding: 2px 10px; } /*-- write specific for blog block --*/

(attached screenshot 1,2 points)

Thanks

zymphonies-dev’s picture

Status: Needs review » Needs work

changing status to "need work"

themestar’s picture

Status: Needs work » Needs review

Thanks shanidkv,
Fixed CSS issue please check

zymphonies-dev’s picture

confirmed,
theme looks good now

bryanbraun’s picture

Status: Needs review » Needs work

Everything seemed to pass the automated tests I ran. I looked through your Code manually and found a few things:

  1. In README.txt it looks like there's a spelling mistake. You refer to "spurfish" module and I think you mean "Superfish"
  2. You probably ought to set your file permissions to more restrictive settings. It looks everything is set at 755 (or rwxr-xr-x). I'd recommend having all files set to 644 (rw-r--r--), and only keep the directories set to 755. See also this comment
  3. The LICENSE.txt file shouldn't be in there since it is automatically added during packaging. See item 4.1 on http://drupal.org/node/1587704

The theme looks nice... it will be a great addition to the theme directory. Keep up the good work.

themestar’s picture

thanks for your feedback
i will fix these issues

gillisig’s picture

You might want to consider another name. There is already a theme called Professional Theme (http://drupal.org/project/professional_theme)

Also your project page is not very detailed, it would be helpful if you gave a quick brief of what is different about your theme compared to the others, even if it involves counting up its visual features.

themestar’s picture

@bryanbraun
i have fixed all issues. please check.

@s2sz
can i use name "Max Professional Theme". just i named as "Max"

gillisig’s picture

Yeah probably you can use Max Professional Theme. although I am no expert since I am in the process of releasing my first theme here as well so maybe others want to pitch in whether that is ok or not :)

themestar’s picture

if i change project name. i have to create new sandbox project and upload files. maybe better to keep this same name ( "Drupal Professional Theme" ). and i have fixed all issues. please tell me your suggestion about naming. or shall i change to "need review" status

Danny Englander’s picture

Hi themestar, that's only the case for full projects as far as renaming goes and it's really the alias that counts. You can rename your project when it's in the sandbox state by simply editing your project title and there is nothing dependent on the URL / alias right now. Once your project is promoted to full, you then determine an alias for the project i.e. max_professional_theme or whatever. I just went through that and I actually renamed my sandbox a few times and did not need to create a new project. Once my project got approved and I had full project promotion rights, I then had to choose the final resting place of my theme by then selecting a specific unchangeable alias.

Danny Englander’s picture

Issue summary: View changes

Description changes

themestar’s picture

Status: Needs work » Needs review

I have changed theme name to "Max Professional Theme".
please check

peterx’s picture

Hello themestar,
Nice use of red, white, and black.

Does your theme pass any accessibility standards, like WAI? If it does, it is worth mentioning on your project page.

Do you have a demo page?

themestar’s picture

Yes,
i followed all drupal standerds and compiled in http://ventral.org/.
presently i dont have any domain & server. so i will try to upload in my friends server

monymirza’s picture

Status: Needs review » Needs work
themestar’s picture

Status: Needs work » Needs review

Fixed, please check

Danny Englander’s picture

Status: Needs review » Needs work

Hi themestar here are a few items for feedback. If you are changing the name of the theme, you should probably edit the info file.

Right now it says:
drupal_professional_theme.info

...should it be:
max_professional_theme.info -- You may also want to update your default logo and the theme description in the info file. You will probably want to change the name of your theme folder to match this as well.

I am also getting a Javascript error relating to Superfish. I am not sure why you have the call to Superfish in your theme. The Readme says to install the Superfish module. That module takes care of any calls to Superfish other than downloading the library itself so I think your code in script.js is superflous.

The Readme also mentions:

slideshow ready for use

I did not see any theme setting or otherwise for a slideshow after I installed your theme.

I also think your project page lacks any detail, description or depth. You might want to take a look at these:

Danny Englander’s picture

Note, I am not sure if this has been mentioned but you may want to consider:

#1410826: [META] Review bonus - by reviewing other's project applications, it will help with your own and it's also a good learning experience.

themestar’s picture

ok thanks,
i will review other's project applications

themestar’s picture

Issue summary: View changes

theme name changes

LaelMoon’s picture

Issue summary: View changes

Create a new issue in the Project Applications queue.

PA robot’s picture

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.

PA robot’s picture

Issue summary: View changes

i edit this nod, By my mistake. revert.